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

Disable progress bar while linting with --hide-progress #1769

Merged
merged 10 commits into from
Aug 29, 2022

Conversation

mashehu
Copy link
Contributor

@mashehu mashehu commented Aug 25, 2022

No description provided.

nf-core_bot and others added 2 commits August 25, 2022 16:40
@codecov
Copy link

codecov bot commented Aug 25, 2022

Codecov Report

Merging #1769 (e30ebb4) into dev (d404374) will increase coverage by 0.06%.
The diff coverage is 88.46%.

❗ Current head e30ebb4 differs from pull request most recent head f4449ea. Consider uploading reports for the commit f4449ea to get more accurate results

@@            Coverage Diff             @@
##              dev    #1769      +/-   ##
==========================================
+ Coverage   69.22%   69.28%   +0.06%     
==========================================
  Files          59       59              
  Lines        7142     7147       +5     
==========================================
+ Hits         4944     4952       +8     
+ Misses       2198     2195       -3     
Impacted Files Coverage Δ
nf_core/modules/modules_json.py 74.65% <ø> (ø)
nf_core/modules/modules_repo.py 84.21% <66.66%> (+1.35%) ⬆️
nf_core/__main__.py 53.50% <100.00%> (+0.72%) ⬆️
nf_core/lint/__init__.py 74.18% <100.00%> (+0.10%) ⬆️
nf_core/modules/lint/__init__.py 82.56% <100.00%> (+0.08%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@mashehu mashehu requested a review from ewels August 25, 2022 15:27
@mashehu mashehu added this to the 2.5 milestone Aug 26, 2022
Copy link
Member

@ewels ewels left a comment

Choose a reason for hiding this comment

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

Awesome!

What do you think about putting this at the top level instead, alongside --verbose etc? As it is (a) uncommon to use and (b) used by many commands.

Also, it would be nice to be able to set these with environment variables (much cleaner for this use case for example). Can do that for all options with auto_envvar_prefix, or just this one with envvar. See click docs

@mashehu
Copy link
Contributor Author

mashehu commented Aug 29, 2022

Okay, auto_envvar_prefix is now enabled with prefix NFCORE (I think NF_CORE would have been too confusing, because _ separates subcommands).

I don't know how to nicely pass the option from the main command to the subcommands, so it is still on an individual command level. There is also no global option for rich to disable progress bars, but only on an per instance basis, therefore I think this approach makes it a bit more future proof.

@mashehu mashehu force-pushed the disable-lint-progress-bar branch from 485c2ab to e30ebb4 Compare August 29, 2022 09:45
@ewels
Copy link
Member

ewels commented Aug 29, 2022

I don't know how to nicely pass the option from the main command to the subcommands

You use click "context" for this, see the click docs. But if we already have it working then it's not a particularly high priority (especially when it's not used on all commands).

Copy link
Member

@ewels ewels left a comment

Choose a reason for hiding this comment

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

Happy with this, thanks! 🙌🏻

Comment on lines +32 to +33
NFCORE_LINT_HIDE_PROGRESS: true
NFCORE_MODULES_LINT_HIDE_PROGRESS: true
Copy link
Member

Choose a reason for hiding this comment

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

Hah, hadn't thought about this - that the env var would be different for different commands.

Ok so then this would be a vote for using an explicit env="HIDE_PROGRESS" on the commands so that we could have one env var that covers the entire tool (or using it on root in one place).

Again low priority. Basically this current setup is fine, might be nice to refactor one day (especially if we add more progress bars) but no needed.

@mashehu mashehu merged commit 6df2717 into nf-core:dev Aug 29, 2022
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.

2 participants