Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

chore: re-enable ruff warnings#961

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@wyardley
Copy link
Contributor

@wyardleywyardley commentedJun 17, 2024
edited
Loading

Having the--diff flag inruff check seems to suppress failures from happening in CI. Remove the--diff flag, and then ignore or adjust to resolve the remainingruff failures. This will help prevent new ruff errors from going unnoticed and getting checked in, at the expense of a little bit of clutter in the code.

Note: this can be merged with the two atomic commits, or squashed as one.

We should probably do this after#957 goes in (may require a little conflict resolution on one file)

@wyardleywyardleyforce-pushed thewyardley/ruff_check_fix_ci branch from9fa20a9 to33c00b9CompareJune 17, 2024 23:17
DEFAULT_ENV_TOKEN_NAME="HVCS_TOKEN"# noqa: S105

def__init__(self,remote_url:str,*args:Any,**kwargs:Any)->None:
def__init__(self,remote_url:str,*args:Any,**kwargs:Any)->None:# noqa: ARG002
Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

If you think we can remove**kwargs here, I think we could also use it on the various specific vcs systems that inherit?

I don't think just renaming to**_kwargs is the right fix, but that would also shut up ruff, I think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This is a dumb error from ruff. yes it is needed as stated above, I'm fine with using the underscore rather than a bunch of ignores.

@wyardleywyardleyforce-pushed thewyardley/ruff_check_fix_ci branch from33c00b9 tocaa6000CompareJune 17, 2024 23:27
@codejedi365codejedi365 self-requested a reviewJune 18, 2024 05:11

# Execute in mocked environment expecting a GitlabUpdateError to be raised
withcreate_release_patch,edit_release_notes_patch,get_release_by_id_patch:
withcreate_release_patch,edit_release_notes_patch,get_release_by_id_patch:# noqa: SIM117
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

What is this about... seems like aruff bug. gahh i hate using <1.0 released software.

Copy link
ContributorAuthor

@wyardleywyardleyJun 18, 2024
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It's the nestedwith; the alternative they suggest is to dowith A() as a, B() as b:, but of course that would look pretty messy here, since the first line is 3 separate items....

https://docs.astral.sh/ruff/rules/multiple-with-statements/

If the raises could be a decorator, maybe that would look better, but I think just ignoring it is probably fine here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Well the doc doesn't consider this situation for creation of the context manger is multiple lines and prior to the with statement.

Yes, ignore is fine here.

@wyardleywyardleyforce-pushed thewyardley/ruff_check_fix_ci branch fromc153971 to0ea9062CompareJune 18, 2024 05:34
Copy link
Contributor

@codejedi365codejedi365 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

see comments above.

@codejedi365codejedi365force-pushed thewyardley/ruff_check_fix_ci branch from0ea9062 todb73750CompareJune 18, 2024 05:39
@wyardleywyardleyforce-pushed thewyardley/ruff_check_fix_ci branch fromdb73750 tof7be6faCompareJune 18, 2024 05:46
Having the `--diff` flag in `ruff check` seems to suppress failures fromhappening in CI
@wyardleywyardleyforce-pushed thewyardley/ruff_check_fix_ci branch fromf7be6fa toce5a891CompareJune 18, 2024 05:52
@wyardleywyardley marked this pull request as ready for reviewJune 18, 2024 05:54
Copy link
Contributor

@codejedi365codejedi365 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Looking better

Co-authored-by: codejedi365 <codejedi365@users.noreply.github.com>
@wyardleywyardleyforce-pushed thewyardley/ruff_check_fix_ci branch fromce5a891 toc3c0603CompareJune 18, 2024 05:56
@codejedi365codejedi365 merged commitd666502 intopython-semantic-release:masterJun 18, 2024
@wyardleywyardley deleted the wyardley/ruff_check_fix_ci branchJune 20, 2024 20:27
@wyardley
Copy link
ContributorAuthor

just noting here, I poked at this some more to create the bug report upstream, and the behavior doeskind of make sense... it's just confusing
ruff --diff will exit 0 if there are no diffs (proposed changes) reported and exit 1 if there are. However, there are errors / findings that do not, or cannot, generate changes to files, but are still errors.

It may be worth addingruff --diff as part of the pipeline somewhere to display the changes (if we want) - however, using it the way we were originally probably doesn't make sense.

@codejedi365
Copy link
Contributor

Thanks for looking into this, definitely is a weird result. Either way all I want is it to display where the error is located. The diff is a nice to have where it shows what to fix but I can also look up the rule for where it indicates an error exists.

@wyardley
Copy link
ContributorAuthor

Yeah, re-reading the docs for--diff and for--fix-only (which it notes is implied), it actually does make sense:

      --diff          Avoid writing any fixed files back; instead, output a diff for each changed          file to stdout. Implies `--fix-only`[...]      --fix-only          Apply fixes to resolve lint violations, but don't report on leftover          violations. Implies `--fix`. Use `--no-fix-only` to disable or `--unsafe-fixes`          to include unsafe fixes

I think in this case "report on" also implies the exit status as well, in other words, because--diff implies--fix-only, it is expected that it doesn't report non-fixable findingsand exits 0 for non-fixable findings. So the interaction of flags is a little unexpected.

I agree that things are fine as it is now. It would be nice to have a way to both report the fixable changes as a diff, and also check for findings, with a single command, but seems like not possible as-is. Short of some additional notes in the docs, or adding some additional flags, might be difficult to fix as well.

codejedi365 reacted with thumbs up emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@codejedi365codejedi365codejedi365 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@wyardley@codejedi365

[8]ページ先頭

©2009-2025 Movatter.jp