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

Don't show spinner with verbose output #160

Merged
merged 7 commits into from
Jan 10, 2023

Conversation

danielhollas
Copy link
Contributor

@danielhollas danielhollas commented Nov 23, 2022

When verbose output is enabled via the -v option, the spinner interferes with the extra output.

Closes #146

When verbose output is enabled via the `-v` option,
the spinner mucks the extra output.

Closes aiidalab#146
@danielhollas danielhollas self-assigned this Nov 23, 2022
@danielhollas danielhollas added the enhancement New feature or request label Nov 23, 2022
@codecov
Copy link

codecov bot commented Nov 23, 2022

Codecov Report

Base: 86.63% // Head: 86.72% // Increases project coverage by +0.08% 🎉

Coverage data is based on head (a5ac96b) compared to base (9a1b522).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #160      +/-   ##
==========================================
+ Coverage   86.63%   86.72%   +0.08%     
==========================================
  Files           9        9              
  Lines         898      904       +6     
==========================================
+ Hits          778      784       +6     
  Misses        120      120              
Flag Coverage Δ
py-3.10 86.61% <100.00%> (+0.08%) ⬆️
py-3.8 86.57% <100.00%> (+0.09%) ⬆️
py-3.9 86.68% <100.00%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aiidalab_launch/__main__.py 79.93% <ø> (ø)
aiidalab_launch/util.py 85.93% <100.00%> (+0.69%) ⬆️

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@unkcpz unkcpz added this to the 2023.1 milestone Nov 25, 2022
Copy link
Member

@yakutovicha yakutovicha left a comment

Choose a reason for hiding this comment

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

One little question.


# Don't show spinner if verbose output is enabled
level = logging.getLogger().getEffectiveLevel()
if level != 0 and level < 40:
Copy link
Member

Choose a reason for hiding this comment

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

I am not very experienced with this particular code, but the level < 40 condition seems strange. If a higher number means "more verbose", then there should be no upper limit, no?

Copy link
Member

Choose a reason for hiding this comment

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

@danielhollas Is the user allowed to change the level? If I understand correctly, the level is increased by adding more v? If this is the case, it is better to update the help information of -v.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yakutovicha actually, the level indicates severity, so lower number means less severe, hence more verbose. See

https://docs.python.org/3/library/logging.html#logging-levels

The default level is logging.ERROR which is 40. I have replaced the hardcoded numbers with the proper variables from the logging module, thanks for pointing this out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the user allowed to change the level? If I understand correctly, the level is increased by adding more v? If this is the case, it is better to update the help information of -v.

@unkcpz yes, you can adjust the level based on the number of vs. The current help says

Provide this option to increase the output verbosity of the launcher.

@yakutovicha @unkcpz do you have suggestion how to improve this? I cannot figure out how to word this concisely.

Copy link
Member

@unkcpz unkcpz Jan 6, 2023

Choose a reason for hiding this comment

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

I am sure no good at wording docs. I check the python cmdline doc https://docs.python.org/3/using/cmdline.html#cmdoption-1. We can assume the user only uses -v and -vv and for -vv giving the detail of what will be printed more, phrased like "When given twice (-vv), .."

@danielhollas
Copy link
Contributor Author

@yakutovicha @unkcpz I've refactored the code a bit, the original approach wasn't handling newlines correctly. I've also tried to improve the description of the --verbose cmdline option. Can you take a look again?

Copy link
Member

@yakutovicha yakutovicha left a comment

Choose a reason for hiding this comment

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

Almost good to go for me. Just one little suggestion.

aiidalab_launch/util.py Outdated Show resolved Hide resolved
@danielhollas
Copy link
Contributor Author

@unkcpz if this looks good to you, could you please merge and then publish a new version of aiidalab-launch? Thanks!

@yakutovicha yakutovicha merged commit 4d63757 into aiidalab:main Jan 10, 2023
@danielhollas danielhollas deleted the no-spinner branch January 10, 2023 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable spinner in debugging mode
3 participants