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

precommit: enableend-of-file-fixer#1920

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

Merged

Conversation

Borda
Copy link
Contributor

let's formalize suggestion from#1888 (comment)

@BordaBorda mentioned this pull requestMay 30, 2024
Copy link
Member

@EliahKaganEliahKagan left a comment
edited
Loading

Choose a reason for hiding this comment

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

Although I think the underlying concept here is good, either all or all but one of the changes insidetest/fixtures should be omitted.

Most files intest/fixtures are test data. Test data should rarely if ever modified for stylistic reasons, because it can break tests, including in ways whose impact would only occur in the case of a regression that a test would otherwise catch. In other words, changing test data without fully and carefully examining exactly how that change might relate toeach use of the data in the tests can cause tests to break in ways that running the tests does not reveal.

The changes heredo break the tests. Fortunately, they break them in a way that is easy to catch: a test fails that is supposed to pass. They may or may notalso break the tests in other ways that are not easy to catch, i.e., that don't affect their status now, but that make the tests pass in slightlymore circumstances than they should, thereby weakening them against future inadvertent bugs.

In the case oftest/fixtures/.gitconfig, it may be worthwhile to look into this and maybe make the change anyway, since that file is not just used as a fixture: it is also used to configure CI. In the case of all other files in thetest/fixtures directory that are modified here, I do not think there is sufficient benefit to justify the added time and effort that it would take to achieve a reasonable level of confidence that the changes would not cause any tests to continue passing in any circumstances under which they should start failing.

Although no*.py files insidetest/fixtures are changed here, those files, while they are considered fixtures, are not test data, so I think it is fine to perform these kinds of stylistic checks and fixes on them. Other files intest/fixtures, with thepossible exception oftest/fixtures/.gitconfig, really should not be subject to these kinds of checks or receive these kinds of changes.

See also#1888 (comment).

The changes outside oftest/fixtures, including the automated changes indoc/source, all look good to me.

Copy link
Member

@ByronByron left a comment

Choose a reason for hiding this comment

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

Thanks for the initiative!

I am now officially setting this PR to a state that indicates that some modifications are needed.

Borda reacted with eyes emoji
@Borda
Copy link
ContributorAuthor

The changes outside oftest/fixtures, including the automated changes indoc/source, all look good to me.

reverted the changes intests/fixtures and added this folder to be ignored

EliahKagan reacted with thumbs up emoji

@BordaBorda requested review fromByron andEliahKaganJuly 16, 2024 10:37
Copy link
Member

@EliahKaganEliahKagan left a comment

Choose a reason for hiding this comment

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

LGTM

@Borda
Copy link
ContributorAuthor

@Byron mind have a look again pls

Copy link
Member

@ByronByron left a comment
edited
Loading

Choose a reason for hiding this comment

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

Thanks a lot!

EliahKagan reacted with thumbs up emoji
@ByronByron merged commit3d3c86c intogitpython-developers:mainJul 17, 2024
26 checks passed
@BordaBorda deleted the precommit/end-of-file-fixer branchJuly 17, 2024 09:46
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@ByronByronByron approved these changes

@EliahKaganEliahKaganEliahKagan approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@Borda@Byron@EliahKagan

[8]ページ先頭

©2009-2025 Movatter.jp