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

DM-23173: change to binary operators at beginning of new line #125

Merged
merged 3 commits into from Apr 9, 2020

Conversation

r-owen
Copy link
Contributor

@r-owen r-owen commented Apr 9, 2020

Also:

  • Update to use f strings
  • Eliminate "u" strings (the "u" is meaningless in Python 3)
  • Standardize on sys.stderr.write instead of print(..., file=sys.stderr)
  • Change remaining % formatted log messages to deferred formatting (most already were)

and fix the resulting flake8 warnings, all of which
were for strings in cmdLineTask.py that I changed to f strings.
and sys.stderr.write instead of print(..., file=sys.stderr)
(the old code was a mix).
@r-owen r-owen requested a review from timj April 9, 2020 15:31
Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

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

I don't like the change to write and I don't understand why it's been done. write is a lower level interface than print.

@@ -1039,7 +1038,7 @@ def write(self, showStr):
matHistory = re.search(r"^(?:config.)?(.+)?", showArgs)
globPattern = matHistory.group(1)
if not globPattern:
print("Please provide a value with --show history (e.g. history=*.doXXX)", file=sys.stderr)
sys.stderr.write("Please provide a value with --show history (e.g. history=*.doXXX)\n")
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this at all. What's the justification given that file=sys.stderr is a good option already.

Copy link
Contributor Author

@r-owen r-owen Apr 9, 2020

Choose a reason for hiding this comment

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

The justification is that the existing code is a mix of the two styles (even within the same module). I feel write is a bit cleaner, though it does risk forgetting the final \n. In any case I think we should pick one and stick to it.

Copy link
Contributor Author

@r-owen r-owen Apr 9, 2020

Choose a reason for hiding this comment

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

Since you prefer print and I don't have strong feelings either way I updated the code to use print everywhere. It was all in this one module.

@r-owen r-owen merged commit 224c443 into master Apr 9, 2020
@r-owen r-owen deleted the tickets/DM-23173/pipe_base branch April 9, 2020 18:22
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.

None yet

2 participants