Adopting ruff formatter#7930
Conversation
|
I suppose I could run Ruff local to fix the linting issues...what do you think @sydney-runkle? |
|
Hi @Luca-Blight, Looks good! Yeah, go ahead and run the linter locally to fix the failing checks. I'll take another look after you do that. Thanks 🌟 |
| raise PydanticUserError( | ||
| f"A non-annotated attribute was detected: `{var_name} = {value!r}`. All model fields require a " | ||
| f"type annotation; if `{var_name}` is not meant to be a field, you may be able to resolve this " | ||
| f'A non-annotated attribute was detected: `{var_name} = {value!r}`. All model fields require a ' | ||
| f'type annotation; if `{var_name}` is not meant to be a field, you may be able to resolve this ' | ||
| f"error by annotating it as a `ClassVar` or updating `model_config['ignored_types']`.", | ||
| code='model-field-missing-annotation', |
There was a problem hiding this comment.
This change also seems odd to me, as it's not changing the last line. I wonder what happens when we use:
[tool.ruff.flake8-quotes]
multiline-quotes = 'double'
In the pyproject.toml
There was a problem hiding this comment.
Actually, it should be 'double' by default, so I don't think that'll change anything. This modification still strikes me as odd though.
There was a problem hiding this comment.
Yea good eye there. Let me try the change out and see what happens.
There was a problem hiding this comment.
Btw, your diligence is impressive and I appreciate it.
There was a problem hiding this comment.
It looks like
[tool.ruff.flake8-quotes]
multiline-quotes = 'double'
is already implemented on line 179:
[tool.ruff]
line-length = 120
extend-select = ['Q', 'RUF100', 'C90', 'UP', 'I', 'D', 'T']
extend-ignore = ['D105', 'D107', 'D205', 'D415']
flake8-quotes = {inline-quotes = 'single', multiline-quotes = 'double'}
mccabe = { max-complexity = 14 }
isort = { known-first-party = ['pydantic', 'tests'] }
target-version = "py37"
extend-exclude = ['pydantic/v1', 'tests/mypy/outputs']
There was a problem hiding this comment.
Good catch. I'll leave this as unresolved for now given that I'm still curious about the change the ruff formatter made, but I don't think it's a problem in terms of the flake8-quotes configuration.
There was a problem hiding this comment.
This strikes me as a bug -- we should be using double quotes for multi-line strings regardless of the formatter quote setting. Will look into it shortly and report back.
There was a problem hiding this comment.
The last line doesn't get changed because changing the quote style would require escaping the '. See my other comment below.
There was a problem hiding this comment.
Sorry, as usual, Micha is right -- this is an implicit concatenation, not a multiline string. So it's trying to use single quotes for each segment, and then skipping those that contain a single quote.
There was a problem hiding this comment.
Ah, I see. This happened a few times with implicit concatenations, and I thought the pattern was odd at first glance bc it always seemed to be skipping the last line, but that seems to always be where we have a single quote based on the nature of many of our strings. Thanks!
| with pytest.raises( | ||
| NameError, | ||
| match="Private attributes must not use dunder names;" " use a single underscore prefix instead of '__foo__'.", | ||
| match='Private attributes must not use dunder names;' " use a single underscore prefix instead of '__foo__'.", |
There was a problem hiding this comment.
Clearly it's having a hard time with this as well 😆
There was a problem hiding this comment.
Yea that is weird. I wonder what it's tripping on...
There was a problem hiding this comment.
So, what's happening here is that it's trying to convert each segment to single quotes (as per quote-style = 'single'). It's succeeding for the first segment in this implicit concatenation, but it avoids changing the second segment, because that segment itself contains a single quote (and so that single quote would need to be escaped).
Arguably we should be making that decision across the entire implicit concatenation (i.e., looking across all segments) to avoid these mixed quotes. I'll file an issue and we'll discuss.
There was a problem hiding this comment.
Ruff changes the quotes of the first string but not of the second because it would require escaping the '. Is it unexpected that Ruff uses different quotes for different parts of the implicit concatenated string? Would you prefer if it would change the quotes consistently for the entire implicit concatenated string?
There was a problem hiding this comment.
Ah, thanks for the explanation. Clearly I missed the pattern of escaping ' that was at the root of many of my questions.
I do feel like changing quotes consistently for an entire implicit concatenated string might look a bit cleaner, but I'm not adamant about the change. Excited to hear what others think.
@charliermarsh, thanks for filing an issue 👍
There was a problem hiding this comment.
@charliermarsh could you link the issue here? I may have just missed it, but didn't see it at a first glance
There was a problem hiding this comment.
Apologies -- just filed it here: astral-sh/ruff#8265
|
Thanks for your work on this 💯. Hopefully we can resolve the few odd formatting patterns that we're seeing tomorrow and get this across the line 😄. |
|
Thanks for your super prompt support. Looks like we'll have this across the line in no time 🚀 ! |
sydney-runkle
left a comment
There was a problem hiding this comment.
LGTM 👍
The only thing I'm still seeing is the implicit concatenation changes that we discussed already, which I don't think we have to address in this PR, but can continue discussion with ruff and change this down the line.
@Luca-Blight, great work!
I noticed some letter swapping in some of the files such as rf -> fr and br -> rb. |
|
I see it's changing |
| # insert_assert(log) | ||
| assert log == [ | ||
| 'json enter input={"a": 2} kwargs={\'strict\': None, \'context\': {\'c\': 2}}', | ||
| "json enter input={\"a\": 2} kwargs={'strict': None, 'context': {'c': 2}}", |
There was a problem hiding this comment.
@charliermarsh if you are looking for feedback, I'd think that it would be preferable to use single quotes on the outer level when single quotes are preferred and the string contains both single and double quotes. But I definitely don't care enough to hold off on merging just for this issue 😄
There was a problem hiding this comment.
Yeah I agree — this one struck me as odd. Gonna look into it.
Change Summary
Ruff is now the sole linter 👍
Files Altered: ['Makefile', 'docs/contributing.md', 'pyproject.toml', 'pdm.lock' ]
Notes:
For pyproject.toml file, I removed the last element('E501') of line 180 given black's removal but I'm not totally sure what this line is for.
Related Issue Number
Fix #7914
Checklist