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.

Sign up for GitHub

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

Jump to bottom

refact: Change function structure to use dicts not lists #12

Merged
merged 17 commits into from
Aug 22, 2024
Merged

refact: Change function structure to use dicts not lists #12

merged 17 commits into from
Aug 22, 2024

Conversation

jvfe
Copy link
Collaborator

@jvfe jvfe commented Aug 15, 2024

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 requested a review from muffato August 15, 2024 17:51
muffato
muffato reviewed Aug 15, 2024
View reviewed changes
nf_core/components/components_utils.py Outdated Show resolved Hide resolved
nf_core/components/components_utils.py Outdated Show resolved Hide resolved
nf_core/components/components_utils.py Outdated Show resolved Hide resolved
nf_core/components/components_utils.py Outdated Show resolved Hide resolved
@jvfe jvfe requested a review from muffato August 21, 2024 16:33
muffato
muffato reviewed Aug 21, 2024
View reviewed changes
Copy link
Member

@muffato muffato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one possible review of your changes.

You've started using get but because you still have if statements, I think most of them are unnecessary and could be regular dict[key].

nf_core/components/components_utils.py Outdated Show resolved Hide resolved
nf_core/components/install.py Outdated Show resolved Hide resolved
nf_core/modules/modules_json.py Outdated Show resolved Hide resolved
nf_core/modules/modules_json.py Outdated Show resolved Hide resolved
nf_core/components/components_utils.py Outdated Show resolved Hide resolved
nf_core/components/install.py Outdated Show resolved Hide resolved
nf_core/components/install.py Outdated Show resolved Hide resolved
nf_core/modules/modules_json.py Outdated Show resolved Hide resolved
nf_core/modules/modules_json.py Outdated Show resolved Hide resolved
nf_core/modules/modules_json.py Outdated Show resolved Hide resolved
@muffato
Copy link
Member

muffato commented Aug 21, 2024

(another review is coming, wait a sec)

@muffato muffato mentioned this pull request Aug 21, 2024
Merged
4 tasks
@jvfe jvfe requested a review from muffato August 22, 2024 17:52
muffato
muffato approved these changes Aug 22, 2024
View reviewed changes
nf_core/components/components_utils.py Outdated Show resolved Hide resolved
jvfe and others added 2 commits August 22, 2024 17:31
@jvfe @muffato
refact: Change UserWarning message
2b9a573 
Co-authored-by: Matthieu Muffato <mm49@sanger.ac.uk>
@jvfe
style: Run ruff format
e55c74d
@jvfe jvfe merged commit 4b9fea5 into fix/1927 Aug 22, 2024
60 of 61 checks passed
@jvfe jvfe deleted the refact/change_structure branch August 23, 2024 12:30
@jvfe jvfe mentioned this pull request Aug 23, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@muffato muffato muffato approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jvfe @muffato