Use ruff's formatter #177

henriquegemignani · 2024-07-14T15:02:14Z

No description provided.

codecov · 2024-07-14T15:04:26Z

Codecov Report

Attention: Patch coverage is 65.62500% with 44 lines in your changes missing coverage. Please review.

Project coverage is 76.98%. Comparing base (6943fc8) to head (fec15d1).
Report is 6 commits behind head on main.

Files	Patch %	Lines
...rc/mercury_engine_data_structures/formats/bmtre.py	0.00%	14 Missing ⚠️
...mercury_engine_data_structures/file_tree_editor.py	50.00%	4 Missing ⚠️
src/mercury_engine_data_structures/common_types.py	75.00%	3 Missing ⚠️
...rc/mercury_engine_data_structures/formats/bmsad.py	85.71%	2 Missing ⚠️
...rc/mercury_engine_data_structures/formats/brfld.py	33.33%	2 Missing ⚠️
...ercury_engine_data_structures/formats/gui_files.py	66.66%	2 Missing ⚠️
src/mercury_engine_data_structures/formats/pkg.py	50.00%	2 Missing ⚠️
src/mercury_engine_data_structures/game_check.py	33.33%	2 Missing ⚠️
...pyinstaller/hook-mercury_engine_data_structures.py	0.00%	1 Missing ⚠️
src/mercury_engine_data_structures/cli.py	0.00%	1 Missing ⚠️
... and 11 more

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #177   +/-   ##
=======================================
  Coverage   76.98%   76.98%           
=======================================
  Files          76       76           
  Lines        3494     3494           
=======================================
  Hits         2690     2690           
  Misses        804      804

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Miepee · 2024-07-14T15:13:41Z

src/mercury_engine_data_structures/formats/blsnd.py

-        Const(0x000B0001, Hex(Int32ul)),
-        Const(0x000C0001, Hex(Int32ul))
+    "version"
+    / IfThenElse(


Ruff makes these look rather ugly :(

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?

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.

no this is really bad imo

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.

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?

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"?

ThanatosGit

Should the marked lines also be excluded from formatting?

ThanatosGit · 2024-07-16T15:40:50Z

src/mercury_engine_data_structures/type_lib.py

    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(


ThanatosGit · 2024-07-16T15:41:11Z

src/mercury_engine_data_structures/construct_extensions/alignment.py

@@ -112,13 +112,14 @@ def PrefixedAllowZeroLen(lengthfield, subcon, includelengthfield=False):
    return FocusedSeq(
        "prefixed",
        "len" / Peek(lengthfield),
-        "prefixed" / Prefixed(
+        "prefixed"
+        / Prefixed(


ThanatosGit · 2024-07-16T15:41:24Z

src/mercury_engine_data_structures/formats/bmsad.py

+            Struct(
+                "type" / StrId,
+                "value"
+                / Switch(


henriquegemignani · 2024-07-16T16:03:49Z

I saw these when reviewing it myself and thought they were small enough to not bother.

ThanatosGit · 2024-07-16T17:15:24Z

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.

steven11sjf

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.

steven11sjf · 2024-07-22T07:37:32Z

src/mercury_engine_data_structures/cli.py

@@ -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:


Personal preference but hex escapes look better with A-F capitalized, especially \x.

I'm indifferent, but I don't believe it's configurable

steven11sjf · 2024-07-22T07:41:26Z

src/mercury_engine_data_structures/construct_extensions/alignment.py

@@ -112,13 +112,14 @@ def PrefixedAllowZeroLen(lengthfield, subcon, includelengthfield=False):
    return FocusedSeq(
        "prefixed",


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.

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

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

I'm fine having both.
I prefer the kwargs one, but it is more limiting, particularly having unnamed padding

henriquegemignani requested a review from a team July 14, 2024 15:02

Miepee reviewed Jul 14, 2024

View reviewed changes

henriquegemignani force-pushed the feature/formatting branch from c710586 to f266aba Compare July 16, 2024 08:38

henriquegemignani requested review from duncathan, Miepee and ThanatosGit July 16, 2024 09:02

ThanatosGit reviewed Jul 16, 2024

View reviewed changes

henriquegemignani force-pushed the feature/formatting branch from d21b0f9 to 6055e7e Compare July 22, 2024 07:44

steven11sjf reviewed Jul 22, 2024

View reviewed changes

henriquegemignani force-pushed the feature/formatting branch from 6055e7e to 28e7018 Compare August 20, 2024 09:15

henriquegemignani added 5 commits August 20, 2024 12:35

Skip formatting on a few files

811078f

Fixes for formatter

6990c36

Small changes to improve automatic formatting

00678cb

Use ruff's formatter

7451642

Run the formatter

fec15d1

henriquegemignani force-pushed the feature/formatting branch from 28e7018 to fec15d1 Compare August 20, 2024 09:35

henriquegemignani merged commit afb176d into main Aug 22, 2024
9 of 10 checks passed

henriquegemignani deleted the feature/formatting branch August 22, 2024 09:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ruff's formatter #177

Use ruff's formatter #177

henriquegemignani commented Jul 14, 2024

codecov bot commented Jul 14, 2024 •

edited

Loading

Miepee Jul 14, 2024

henriquegemignani Jul 14, 2024

Miepee Jul 14, 2024

henriquegemignani Jul 14, 2024

duncathan Jul 14, 2024

henriquegemignani Jul 15, 2024

ThanatosGit Jul 15, 2024

henriquegemignani Jul 16, 2024

ThanatosGit left a comment

ThanatosGit Jul 16, 2024

ThanatosGit Jul 16, 2024

ThanatosGit Jul 16, 2024

henriquegemignani commented Jul 16, 2024

ThanatosGit commented Jul 16, 2024

steven11sjf left a comment

steven11sjf Jul 22, 2024

henriquegemignani Jul 22, 2024

steven11sjf Jul 22, 2024

steven11sjf Jul 22, 2024

duncathan Jul 22, 2024

henriquegemignani Jul 22, 2024

		@@ -112,13 +112,14 @@ def PrefixedAllowZeroLen(lengthfield, subcon, includelengthfield=False):
		return FocusedSeq(
		"prefixed",

Use ruff's formatter #177

Use ruff's formatter #177

Conversation

henriquegemignani commented Jul 14, 2024

codecov bot commented Jul 14, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThanatosGit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henriquegemignani commented Jul 16, 2024

ThanatosGit commented Jul 16, 2024

steven11sjf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 14, 2024 •

edited

Loading