-
Notifications
You must be signed in to change notification settings - Fork 1k
lint: fix typos and include lint checking #18553
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
base: main
Are you sure you want to change the base?
Conversation
Supports both `reformat` and `lint`. Signed-off-by: Mike Fiedler <[email protected]>
Signed-off-by: Mike Fiedler <[email protected]>
Also demonstrates the correct syntax for different locations: - Python - Markdown - SCSS Signed-off-by: Mike Fiedler <[email protected]>
@@ -43,7 +43,7 @@ def report_vulnerabilities(request): | |||
vulnerability_reports = request.json_body | |||
except json.decoder.JSONDecodeError: | |||
metrics.increment( | |||
"warehouse.vulnerabilties.error.payload.json_error", tags=["origin:osv"] | |||
"warehouse.vulnerabilities.error.payload.json_error", tags=["origin:osv"] |
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 for us to merge the metrics if this ships?
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.
Sadly there's no way to merge metrics yet.
I investigated and this specific metric hasn't reported at least once in our metrics retention history, nor does it appear on any dashboards.
@@ -128,7 +128,7 @@ def test_lookup_actor_404(self, monkeypatch): | |||
) | |||
requests = pretend.stub( | |||
post=pretend.call_recorder(lambda o, **kw: response), | |||
expception=_requests.exceptions, |
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.
seems odd to me that this doesn't change the behavior of this test
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 noting. Looks like the assertions never care about this specific property, so I can remove these from being set, if they have no impact. I'll remove them completely in a following commit.
I understand the intent here, I want to understand if this will auto correct spelling, or if it will be a failure case in check that needs to be manually resolved. |
@@ -1,6 +1,8 @@ | |||
#!/usr/bin/env bash | |||
set -ex | |||
|
|||
# Fix typos before other changes, as they may affect formatting. | |||
codespell --write |
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 am -1 on this. seems it could be a footgun if an undesirable change to spelling is automatically written.
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.
Happy to remove, whenever there is an ambiguity of spelling, codespell won't change it, rather continue to error out during a lint phase.
Happy to remove if you think this is a serious footgun, but otherwise it felt appropriate to help the user similar to other reformat directives.
Supports both
reformat
andlint
.Ref: https://pypi.org/project/codespell/
Closes #11568
Closes #11884