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 — Add advice to "Files were modified…" error by katrinleinweber · Pull Request #1031 · pre-commit/pre-commit

Conversation

@katrinleinweber

@katrinleinweber katrinleinweber commented May 12, 2019

Copy link
Copy Markdown

Would it make sense to include something like this? If yes, please let me know whether the formatting for a string that should not be too long in the source code, but single-line in the out-put is correct.

Afterwards, I'll:

  • update run_test.py

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@katrinleinweber
Add useful instruction to error message
9e17065
asottile
asottile reviewed May 12, 2019
View reviewed changes
Comment thread pre_commit/commands/run.py Outdated
output.write('Files were modified by this hook.')
output.write('Files were modified by this hook.' \
'Please review them in Git\'s staging area, ' \
'and repeat the commit.')

@asottile asottile May 12, 2019

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.

seems fine to add more context here

maybe something like this?

output.write(
    'Files were modified by this hook.  '
    'Review the changes, `git add`, and repeat the commit.',
)

hmmm, but it's not terribly uncommon for this message to show multiple times with lots of fixers (it's a bit redundant in that case) and now it'll wrap on narrow terminals 🤔

(the change won't be in the staging area, but will need to be added there)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
katrinleinweber reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@asottile

asottile commented May 12, 2019

Copy link
Copy Markdown
Member

tl;dr I'm on the fence, it seems like a good idea but also seems a little verbose?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@katrinleinweber
Improve instructions in error message
d597f34
@katrinleinweber

katrinleinweber commented May 14, 2019

Copy link
Copy Markdown
Author

True. Would a separate, final message be better, to avoid redundancy in case several files are modified?
All reactions

Sorry, something went wrong.

Uh oh!

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

asottile commented May 14, 2019

Copy link
Copy Markdown
Member

True. Would a separate, final message be better, to avoid redundancy in case several files are modified?

Ooh yeah that's a good idea, we already do something similar for --show-diff-on-failure, we'd just need to pass along the "changed files" information somehow.

There's a perf improvement that I've been meaning to make here as well, right now execution looks roughly like this:

- run hook 1
    - diff = git diff
    - run hook
    - diff2 = git diff (compare against diff and report Changed files)
- run hook 2
    - diff = git diff
    - run hook
    - diff2 = git diff (compare against diff and report Changed files)

But it could look like this:

- initial_diff = git diff
- run hook 1 (take initial_diff as a parameter)
    - run hook
    - new_diff = git diff (compare against passed in diff)
   - return the new diff
- run hook 2 (take previous returned diff as a parameter)
    - run hook
    - new_diff = git diff (compare against passed in diff)
   - return the new diff
etc.

with that second patern, it would be easy to compare the initial diff against the last returned diff to see if there was any changes across the whole set of hooks
blueyed 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.
@asottile

asottile commented Aug 22, 2020

Copy link
Copy Markdown
Member

I think the work in #1566 will make this easier to accomplish
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@asottile asottile closed this Aug 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@asottile asottile asottile left review comments

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.

2 participants

@katrinleinweber @asottile
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.