-
Notifications
You must be signed in to change notification settings - Fork 293
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
adding script to fix the style and adding modified/fixed files #591
Conversation
f99c315
to
8e44502
Compare
86163f2
to
d7da974
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gobbleturk can you own this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me, thank you! MaxText is becoming more professional every day =)
Please wait for @rwitten thoughts on changing line length from 125 to 80, I vaguely recall we purposefully changed from 80 to 125 at some point early on
@@ -249,7 +254,10 @@ generated-members= | |||
[FORMAT] | |||
|
|||
# Maximum number of characters on a single line. | |||
max-line-length=125 | |||
max-line-length=80 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is fine, although a significant change. I am very appreciative that the provided script should wrap the lines for me. @rwitten do you have a preference here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick review!
I chose line length=80 because that is the google standard. I can ping the reference offline. On the other hand PEP8 standard is 79 https://peps.python.org/pep-0008/#maximum-line-length
Reviewed and LGTM, but I think you should confirm the line length change from 125 -> 80 |
Rafi wants 125 as the max line length and files here are fixed with 80. I am closing this PR in favor of #596 . |
No description provided.