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

Tools: python script cleanups (functionality unchanged) #3123

Merged
merged 6 commits into from
Oct 12, 2022

Conversation

BrandonPacewic
Copy link
Contributor

@BrandonPacewic BrandonPacewic commented Sep 22, 2022

Aligning the python scripts closer to the PEP-8 style guide & some other cleanups.

Functionality is completely unchanged, these suggestions are purely formatting and readability updates. Hopefully bringing some of this code closer to the incredible standards maintained by this team.

@BrandonPacewic BrandonPacewic requested a review from a team as a code owner September 22, 2022 22:07
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Sep 23, 2022
Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

This seems absolutely reasonable to me; I do like the type annotations. Otherwise, I don't personally have a lot of investment in following PEP-8 for our code, but also I don't mind following it either.

Copy link
Member

@barcharcraz barcharcraz left a comment

Choose a reason for hiding this comment

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

Like nicole I don't really care about pep8 (and, in fact, I find it can sometimes have too many rules), but this seems OK to me.

Type annotations are always good, though I mostly use them just for the tooling support.

@BrandonPacewic
Copy link
Contributor Author

Otherwise, I don't personally have a lot of investment in following PEP-8 for our code, but also I don't mind following it either.

Like nicole I don't really care about pep8 (and, in fact, I find it can sometimes have too many rules), but this seems OK to me.

I agree, following pep8 strictly isn't the best (especially with so many rules). Mainly just wanted to increase consistency.

tools/scripts/print_failures.py Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

Thanks for this cleanup! I wanted to use f-strings when I wrote print_failures.py but didn't know how to deal with the nested quotes (since I'm a Python novice), so I'm glad you've shown us how to. There's an issue with the CRLF replacement - I don't know if that line needs to be reverted, or if it can be adjusted to keep the f-string improvement.

@BrandonPacewic
Copy link
Contributor Author

My mistake for not double checking the backslash problem with f-strings, I have reverted that change for now. The other option would be something like this:

my_string = f"This is a backslash: {chr(92)}"

92 being the ASCII code for a backslash \. With the final product being something like this:

print(f"output: {result['output'].replace(f'{chr(92)}r{chr(92)}n', f'{chr(92)}n')}")

Definitely not the best solution but seems to be necessary to get away from .format() currently.

@StephanTLavavej
Copy link
Member

✅ No C++, so no modules impact! 😸

@StephanTLavavej StephanTLavavej self-assigned this Oct 11, 2022
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 1d8fbec into microsoft:main Oct 12, 2022
@StephanTLavavej
Copy link
Member

Thanks for improving these scripts - and congratulations on your first microsoft/STL commit! 🎉 😸 🐍

@BrandonPacewic
Copy link
Contributor Author

Thanks for improving these scripts - and congratulations on your first microsoft/STL commit! 🎉 😸 🐍

🎉🚀🎉

@BrandonPacewic BrandonPacewic deleted the python-script-cleanup branch October 12, 2022 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants