Apply ruff formatting and safe lint fixes - #1061
Conversation
…igial-test-fields [v5.0] Remove vestigial experimental fields from test fixture
Ran `ruff format` and `ruff check --fix` (safe fixes only, line-length 99) over all Python files in src/ and tests/. Resolved conflicts with the project's flake8 configuration: - Disabled RUF100 during the fix so noqa comments flake8 relies on (D-codes, E501, W505) are preserved. - Hand-wrapped 8 f-strings that exceeded 99 chars after .format() -> f-string conversion. - Added E203 to the flake8 ignore list (standard ruff/black slice-spacing exception). Fixed two autofixes that would have broken the test suite: - Restored the load-bearing import order in tests/utils/fakes/__init__.py (marked # isort: skip_file) to avoid a circular import. - Restored the pytest fixture import in tests/serialization/test_scorers.py (marked # noqa: F401). flake8 is clean and all 1237 tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
b267e8f to
7bcc6e3
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
| return IngestionStatus.build( | ||
| { | ||
| "status": IngestionStatusType.INGESTION_CREATED, | ||
| "errors": self.errors, | ||
| } | ||
| ) |
There was a problem hiding this comment.
Hmmmmmmmmmm...not the most egregious, but feels needlessly verbose. Personally, I don't think we gain any clarity by having the braces on their own lines.
There was a problem hiding this comment.
Fixed in pyproject.toml via skip-magic-trailing-comma.
| properties = Optional( | ||
| List( | ||
| Union( | ||
| [ | ||
| LinkOrElse(GEMDPropertyTemplate), | ||
| SpecifiedMixedList( | ||
| [LinkOrElse(GEMDPropertyTemplate), Optional(Object(BaseBounds))] | ||
| ), | ||
| ] | ||
| ) | ||
| ), | ||
| "properties", | ||
| override=True, | ||
| ) |
There was a problem hiding this comment.
Oof. I hate this. However, it's more an expression of how messy the definition of the serde stuff is generally, laid bare by the complication of the type itself. So not sure there's anything to be done. Ugh.
There was a problem hiding this comment.
I created an alias for LinkOrElse(GEMDPropertyTemplate) and swapped the Union to a |. I think it's more legible.
| | LinkByUID | ||
| | Sequence[PropertyTemplate | LinkByUID | BaseBounds | None] | ||
| ] | ||
| | None = None, |
There was a problem hiding this comment.
I continue to think multi-line type definitions in the middle of a function definition are the work of the devil. But I'm not sure a super long line is much better...
| assert ( | ||
| query.criteria[0].property_templates_filter | ||
| == query_copy.criteria[0].property_templates_filter | ||
| ) |
There was a problem hiding this comment.
I don't know the best option for something like this, but I do know I dislike this method of splitting the assert.
There was a problem hiding this comment.
I reworked this as looping over lambdas, since it there were honestly a lot of characters to track on that line.
| assert expected_data_updates["predictors"][0]["predictor_id"] == str( | ||
| actual_data_updates.predictors[0].uid | ||
| ) |
There was a problem hiding this comment.
This is even worse. If it must split them across lines, at least keep the left- and right-hand operands together.
| session.set_response( | ||
| obj.dump() | ||
| ) # Delete calls get, must return object data internally |
There was a problem hiding this comment.
I don't love wrapping due to a comment, even less so in a test. We should just move this comment to the line above.
| session.set_response( | ||
| obj.dump() | ||
| ) # Delete calls get, must return object data internally |
| assert ( | ||
| next( | ||
| ( | ||
| var | ||
| for var in def1.variables | ||
| if name in var.headers and isinstance(var, IngredientLabelsSetByProcessAndName) | ||
| ), | ||
| None, | ||
| ) | ||
| is not None | ||
| ) |
There was a problem hiding this comment.
This is another example of linting making for terribly unreadable code. Is there some way to tell ruff to not format this line? Or do we just need to refactor it to be shorter?
There was a problem hiding this comment.
Honestly, even the original version doesn't spark joy.
| BadRequest( | ||
| "" | ||
| ), # Attempted POST throws BadRequest because, for example, the template bounds are being narrowed. |
There was a problem hiding this comment.
We should move the comment to make this more readable.
There was a problem hiding this comment.
Maybe if there's some way to automatically do that? Can we get ruff to move comments to the line above if the line is short enough without it?
| assert ( | ||
| "1234" | ||
| == Citrine( | ||
| api_key="1234", scheme="http", host="citrine-testing.fake", port="8080" | ||
| ).session.refresh_token | ||
| ) |
There was a problem hiding this comment.
This line is particularly ugly.
| assert ( | ||
| "1234" | ||
| == Session( | ||
| refresh_token="1234", scheme="ftp", host="citrine-testing.fake", port="8080" | ||
| ).refresh_token | ||
| ) |
Follow-up to the ruff-formatting pass, addressing Austin's review feedback and applying the same fixes to unflagged instances of each concern type. - Add [tool.ruff] config: skip-magic-trailing-comma (collapses single-line collections instead of exploding them) with isort.split-on-trailing-comma disabled for compatibility. - Convert .format() calls on string literals to f-strings. - Drop orphaned `# pragma: no cover` comments left after `pass` removal. - Refactor split-operand/wrapped asserts to keep operands together (extract locals, loop over cases, lambda accessors) and move comments that forced line wraps above their expressions. - gemtables/variables.py: extract the repeated ternary into a module-level `_build_attribute_constraints` helper and hoist the duplicated `_AttributeType`/`_ObjectType`/`_ConstraintType` aliases to module scope. - Restore correct (separate) LinkOrElse instances in the template property definitions; shared Property descriptors break serialization-path assignment. - validate_type: raise ValueError instead of bare Exception. - Cover PvA __iter__ explicitly after switching next(iter(...)) reads to []. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| if data_dict_copy["type"] != type_name: | ||
| raise Exception( | ||
| "Object type must be {}, but was instead {}.".format(type_name, data_dict["type"]) | ||
| raise ValueError( |
There was a problem hiding this comment.
I chose to change the type here because we really shouldn't have been throwing an Exception,
| assert any( | ||
| var | ||
| for var in def1.variables | ||
| if name in var.headers and isinstance(var, IngredientQuantityByProcessAndName) |
There was a problem hiding this comment.
Not formally the same but close enough and much more legible.
| cast = str(team_member) | ||
| assert user.screen_name in cast | ||
| assert all(a in cast for a in team_member.actions) | ||
| assert team.name in cast |
There was a problem hiding this comment.
Seemed like it was actually testing what we cared about plus dropped the operator-split.
| return { | ||
| "type": "ContinuousDimension", | ||
| "descriptor": { | ||
| "type": "Real", | ||
| "descriptor_key": "alpha", | ||
| "units": "", | ||
| "lower_bound": 5.0, | ||
| "upper_bound": 10.0, | ||
| }, | ||
| "lower_bound": 6.0, | ||
| "upper_bound": 7.0, | ||
| } |
There was a problem hiding this comment.
Showed up as a non-binding warning.
3 participants
The only purpose of the PR is running all package lines through ruff so that future formatting issues do not distract from code changes.
PR Type:
Adherence to team decisions