Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refact: Add suggestions from the third round of reviews #15

Merged
merged 19 commits into from
Dec 4, 2024
Merged

Conversation

jvfe
Copy link
Collaborator

@jvfe jvfe commented Sep 16, 2024

Adds suggestions from the third round of reviews

PR checklist

  • This comment contains a description of changes (with reason)
  • CHANGELOG.md is updated
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated

@jvfe jvfe linked an issue Sep 17, 2024 that may be closed by this pull request
3 tasks
* wip: Add bug two

* refact: Try defaulting to a dict

* fix: Dont allow remote to be unbound

* refact: Check if not equal to component type

* refact: Re-add isinstance dict

* fix: Remove mention of component_name

* refact: Add special case for strings following dicts

* test: Try removing branch from test case

* refact: Remove isinstance for str

* refact: Use dict only with get_comps out

* Revert "refact: Use dict only with get_comps out"

This reverts commit 7ebc6dc.

* refact: Check if content has subwfs

* refact: Always set current remote

* refact: Check sha before resetting current_repo

* refact: Do the negative test

* refact: Use another variable for remote

* refact: Simplify check

* refact: Expand check once again

* refact: Roll back to previous check

* fix: self.sha must not be none to reset to none

* Revert "fix: self.sha must not be none to reset to none"

This reverts commit 65aa1c4.

* Try removing the section entirely

* refact: Try moving check to reset

* refact: Remove unnecessary current_sha

* refact: Change check in get_comps

* refact: Set git remote beforehand

* refact: Change indent so previous check is the same

* refact: Remove current repo check

* Revert "refact: Remove current repo check"

This reverts commit a956e04.

* refact: Try using name list

* fix: Remove break in loop

* refactor: Always set current_repo

* fix: Check if name is in updated

* refactor: Remove unused sections and logs
get_genome_annotation is not necessary, given we're using another
subworkflow for the other tests
@jvfe jvfe marked this pull request as ready for review November 30, 2024 19:05
@jvfe jvfe requested a review from muffato November 30, 2024 19:05
@jvfe
Copy link
Collaborator Author

jvfe commented Nov 30, 2024

The two failing test cases are - I believe - unrelated to the feature, and will probably fix themselves once I merge the upstream changes to the fix/1927 branch

tests/subworkflows/test_update.py Show resolved Hide resolved
tests/subworkflows/test_update.py Show resolved Hide resolved
nf_core/components/update.py Show resolved Hide resolved
nf_core/components/update.py Outdated Show resolved Hide resolved
nf_core/components/update.py Outdated Show resolved Hide resolved
nf_core/components/update.py Show resolved Hide resolved
nf_core/components/update.py Outdated Show resolved Hide resolved
nf_core/components/update.py Show resolved Hide resolved
@jvfe jvfe requested a review from muffato December 2, 2024 18:07
@jvfe jvfe merged commit 864eb7f into fix/1927 Dec 4, 2024
80 of 83 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create additional test cases for the feature
2 participants