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

add flagg limit_output for modules update #3047

Merged
merged 32 commits into from
Jul 16, 2024
Merged

add flagg limit_output for modules update #3047

merged 32 commits into from
Jul 16, 2024

Conversation

Joon-Klaps
Copy link
Contributor

PR checklist

Fixes #3045

  • 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

@Joon-Klaps Joon-Klaps changed the title add default limit_output argument for write_diff_file & print_diff add flagg limit_output for modules update Jul 3, 2024
@Joon-Klaps Joon-Klaps marked this pull request as ready for review July 3, 2024 12:03
@Joon-Klaps Joon-Klaps requested a review from mashehu July 3, 2024 12:03
mashehu
mashehu reviewed Jul 3, 2024
View reviewed changes
Copy link
Contributor

@mashehu mashehu left a comment

Choose a reason for hiding this comment

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

Could you write a quick test for this as well?

@mirpedrol mirpedrol force-pushed the add-flag-update-modules branch from d859c09 to 8e782fa Compare July 9, 2024 08:10
@mirpedrol
Copy link
Member

@nf-core-bot fix linting please!

@mashehu
Copy link
Contributor

mashehu commented Jul 9, 2024

sorry for messing with your code. I thought I would just polish it a bit, but seems like I created a bit of a mess. working on it.

@Joon-Klaps
Copy link
Contributor Author

sorry for messing with your code. I thought I would just polish it a bit, but seems like I created a bit of a mess. working on it.

No worries, was procastenating as the testing part was still a bit unclear on how I should approach it so feel free to finish it 😄

@mashehu mashehu requested a review from mirpedrol July 15, 2024 09:26
@mirpedrol
Copy link
Member

@nf-core-bot fix linting

mirpedrol
mirpedrol approved these changes Jul 16, 2024
View reviewed changes
Copy link
Member

@mirpedrol mirpedrol left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! LGTM :)
I have modified the tests a little bit to reduce the code.

  1. Using the caplog pytest fixture to capture the log messages
  2. Removing checks that are covered by other tests, such as assessing if a module was properly updated
  3. Since we know which files are modified when we update the modules and subworkflows, we can assess for the specific logging messages directly.

@mirpedrol mirpedrol merged commit 97fd791 into nf-core:dev Jul 16, 2024
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mashehu mashehu mashehu left review comments

@mirpedrol mirpedrol mirpedrol 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.
4 participants
@Joon-Klaps @mirpedrol @mashehu @nf-core-bot