-
Notifications
You must be signed in to change notification settings - Fork 12
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
Use ruff's formatter #177
Use ruff's formatter #177
Conversation
Const(0x000B0001, Hex(Int32ul)), | ||
Const(0x000C0001, Hex(Int32ul)) | ||
"version" | ||
/ IfThenElse( |
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.
Ruff makes these look rather ugly :(
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.
🤷
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.
Is there a way to tell ruff to exempt these? or is the only fix to just change all of those to use =
syntax instead?
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'm kinda fine with how it looks. Not the best, but it's still usable to not bother trying to make it look different.
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.
no this is really bad imo
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'm strong on needing some sort of auto-formatting though. Especially to keep all the project settings in sync across all repos in the organisation.
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 unsucessfully tried to find some sort of rule for that. Is there anyway to do it without disabling ruff for each of those lines?
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.
black and ruff's formatter don't offer configuration to how the code is formatted. Best I can think of is wrapping the construct definitions in a "don't format this block"?
c710586
to
f266aba
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.
Should the marked lines also be excluded from formatting?
def GetTypeConstruct(self, keyfunc, follow_typedef: bool = True) -> construct.Construct: | ||
return construct.FocusedSeq( | ||
"switch", | ||
"key" / construct.Computed(keyfunc), | ||
"type" / construct.Computed(lambda this: self.get_type(this.key, follow_typedef=follow_typedef).name), | ||
"switch" / construct.Switch( | ||
"switch" | ||
/ construct.Switch( |
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.
.
@@ -112,13 +112,14 @@ def PrefixedAllowZeroLen(lengthfield, subcon, includelengthfield=False): | |||
return FocusedSeq( | |||
"prefixed", | |||
"len" / Peek(lengthfield), | |||
"prefixed" / Prefixed( | |||
"prefixed" | |||
/ Prefixed( |
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.
.
Struct( | ||
"type" / StrId, | ||
"value" | ||
/ Switch( |
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 saw these when reviewing it myself and thought they were small enough to not bother. |
Alright, I leave it to the others who were complaining if they want to change it or not. I'm fine with it how it is. |
d21b0f9
to
6055e7e
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.
I read about 1/3 of it but it's mostly good to me. the hex-escape formatting is the biggest issue I have with it.
@@ -177,7 +177,7 @@ def decode_encode_compare_file(file_path: Path, game: Game, file_format: str): | |||
resource = resource_class.parse(raw, target_game=game) | |||
encoded = resource.build() | |||
|
|||
if raw != encoded and raw.rstrip(b"\xFF") != encoded: | |||
if raw != encoded and raw.rstrip(b"\xff") != encoded: |
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.
Personal preference but hex escapes look better with A-F capitalized, especially \x.
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'm indifferent, but I don't believe it's configurable
@@ -112,13 +112,14 @@ def PrefixedAllowZeroLen(lengthfield, subcon, includelengthfield=False): | |||
return FocusedSeq( | |||
"prefixed", |
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 think that we should choose either the / format or the = format and standardize it across the repo. I'm partial to the = format because it feels more readable to me.
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.
This is probably more of a "we should pick one and manually update code to comply/enforce it on new PRs" than something that ruff can handle
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 think that we should choose either the / format or the = format and standardize it across the repo. I'm partial to the = format because it feels more readable to me.
if we must pick one, it needs to be the /
format. readability is secondary to functionality, and using kwargs (the =
format) does not offer as much functionality
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'm fine having both.
I prefer the kwargs one, but it is more limiting, particularly having unnamed padding
6055e7e
to
28e7018
Compare
28e7018
to
fec15d1
Compare
No description provided.