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

Fix flake8 B907 errors #279

Merged
merged 4 commits into from
Feb 22, 2023
Merged

Conversation

giffels
Copy link
Member

@giffels giffels commented Jan 20, 2023

In recent flake8 tests it complains (B907) about three code lines using the mechanism f'Test="{bla}"' to put the value of a variable into double quotes to be executed in a shell. The documentation suggest to change this into f"Test={bla!r}", which leads to a slightly different output Test='value' instead of Test="value". However, this does not matter for the execution of the concerned commands in a shell.

For the third occuarance I decided to ignore that error, since it mimics the output of a moab command in a unittest, which uses double quotes.

@giffels giffels added the Improvement Code Improvements label Jan 20, 2023
@giffels giffels marked this pull request as ready for review January 20, 2023 11:42
@giffels giffels requested review from a team, eileen-kuehn and mschnepf and removed request for a team January 20, 2023 11:42
mschnepf
mschnepf previously approved these changes Jan 26, 2023
Copy link
Member

@mschnepf mschnepf left a comment

Choose a reason for hiding this comment

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

Looks fine to me 👍

@codecov-commenter
Copy link

codecov-commenter commented Jan 27, 2023

Codecov Report

Base: 98.88% // Head: 98.88% // No change to project coverage 👍

Coverage data is based on head (713cfa3) compared to base (248cd6e).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #279   +/-   ##
=======================================
  Coverage   98.88%   98.88%           
=======================================
  Files          56       56           
  Lines        2328     2328           
=======================================
  Hits         2302     2302           
  Misses         26       26           
Impacted Files Coverage Δ
tardis/adapters/sites/slurm.py 100.00% <ø> (ø)
tardis/plugins/auditor.py 100.00% <100.00%> (ø)

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.

@maxfischer2781
Copy link
Member

maxfischer2781 commented Jan 31, 2023

FWIW, I would be fine with disabling this check. We often want/need a specific kind of quote and formatting and !r is not at all equivalent. For example, it will also escape linebreaks, tabs and similar.

Alternatively, we might want to use shlex.quote when the target is a shell (which should be the case for all executors).

mschnepf
mschnepf previously approved these changes Jan 31, 2023
Copy link
Member

@mschnepf mschnepf left a comment

Choose a reason for hiding this comment

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

Looks fine for me 👍

@maxfischer2781
Copy link
Member

@giffels Just a heads up that @eileen-kuehn is waiting (without rushing) whether you would want to change the PR in response to my comment above.

@giffels
Copy link
Member Author

giffels commented Feb 1, 2023

@giffels Just a heads up that @eileen-kuehn is waiting (without rushing) whether you would want to change the PR in response to my comment above.

I will change the pull request tomorrow. I think I am going to ignore B907 then.

eileen-kuehn
eileen-kuehn previously approved these changes Feb 16, 2023
Copy link
Member

@eileen-kuehn eileen-kuehn left a comment

Choose a reason for hiding this comment

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

Looks good to me, go for it 👍

Copy link
Member

@mschnepf mschnepf left a comment

Choose a reason for hiding this comment

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

Looks fine to me. 👍
Go ahead

@giffels giffels added this pull request to the merge queue Feb 22, 2023
Merged via the queue into MatterMiners:master with commit 4d525de Feb 22, 2023
@giffels giffels deleted the fix-flake-static-test branch February 22, 2023 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Code Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants