GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — precommit: enable `end-of-file-fixer` by Borda · Pull Request #1920 · gitpython-developers/GitPython

Conversation

@Borda

@Borda Borda commented May 30, 2024

Copy link
Copy Markdown
Contributor

let's formalize suggestion from #1888 (comment)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Borda added 2 commits May 30, 2024 12:22
@Borda
precommit: enable end-of-file-fixer
9e67138
@Borda
apply
96e21f0
@Borda Borda mentioned this pull request May 30, 2024
lint: add typos check #1888
Merged
EliahKagan
EliahKagan requested changes May 30, 2024
View reviewed changes

@EliahKagan EliahKagan left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

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

Most files in test/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 to each use of the data in the tests can cause tests to break in ways that running the tests does not reveal.

The changes here do 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 not also 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 slightly more circumstances than they should, thereby weakening them against future inadvertent bugs.

In the case of test/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 the test/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 inside test/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 in test/fixtures, with the possible exception of test/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 of test/fixtures, including the automated changes in doc/source, all look good to me.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@EliahKagan EliahKagan mentioned this pull request May 30, 2024
precommit: enable validate-pyproject #1921
Merged
Byron
Byron requested changes May 31, 2024
View reviewed changes

@Byron Byron left a comment

Copy link
Copy Markdown
Member

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 the initiative!

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Borda reacted with eyes emoji
All reactions
  • 👀 1 reaction
@Borda
exclude: test/fixtures/
366a607
@Borda

Borda commented Jul 16, 2024

Copy link
Copy Markdown
Contributor Author

The changes outside of test/fixtures, including the automated changes in doc/source, all look good to me.

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

EliahKagan reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Borda
Borda requested review from Byron and EliahKagan July 16, 2024 10:37
EliahKagan
EliahKagan approved these changes Jul 16, 2024
View reviewed changes

@EliahKagan EliahKagan left a comment

Copy link
Copy Markdown
Member

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.

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@Borda

Borda commented Jul 16, 2024

Copy link
Copy Markdown
Contributor Author

@Byron mind have a look again pls

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Byron
Byron approved these changes Jul 17, 2024
View reviewed changes

@Byron Byron left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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 a lot!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

EliahKagan reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@Byron
Byron merged commit 3d3c86c into gitpython-developers:main Jul 17, 2024
@Borda
Borda deleted the precommit/end-of-file-fixer branch July 17, 2024 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Byron Byron Byron approved these changes
@EliahKagan EliahKagan EliahKagan approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@Borda @Byron @EliahKagan
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.