-
Notifications
You must be signed in to change notification settings - Fork 192
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
Migrate list command to components #1995
Conversation
def pattern_msg(keywords): | ||
if len(keywords) == 0: | ||
return "" | ||
if len(keywords) == 1: | ||
return f" matching pattern '{keywords[0]}'" | ||
else: | ||
quoted_keywords = (f"'{key}'" for key in keywords) | ||
return f" matching patterns {', '.join(quoted_keywords)}" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm never sure on when it is really necessary to have a method within a method, but this feels like it could (and should) be a standalone method outside the class. I find it particularly confusing that keywords
is used in the scope of this internal method whilst being defined in the enclosing method directly above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
partially fixed in 7a2fa10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it with these levels of inheritance :)
I am not sure if this will be possible in all commands as some of them have different functionalities in ModulesCommand
or SubworkflowsCommand
, but we can try 💪
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to add tests/subworkflows/list
? I didn't see it in the diff
I just looked and I think after this only |
Co-authored-by: Júlia Mir Pedrol <mirp.julia@gmail.com>
…to migrate-sw-list
…w-list # Conflicts: # nf_core/modules/modules_json.py
Codecov Report
@@ Coverage Diff @@
## dev #1995 +/- ##
==========================================
+ Coverage 62.50% 63.37% +0.87%
==========================================
Files 43 44 +1
Lines 4902 5030 +128
==========================================
+ Hits 3064 3188 +124
- Misses 1838 1842 +4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Co-authored-by: awgymer <24782660+awgymer@users.noreply.github.com>
Co-authored-by: Júlia Mir Pedrol <mirp.julia@gmail.com>
What if we used component command classes actually? 👿