Skip to content
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

Present a more user-friendly error if .gitignore is invalid #2414

Merged
merged 1 commit into from
Aug 20, 2021

Conversation

nipunn1313
Copy link
Contributor

Fixes #2359

)
assert result.exit_code == 1
assert result.stderr_bytes is not None
assert b"Invalid git pattern: '!\\n'" in result.stderr_bytes
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the error emitted by pathspec 0.9.0 thanks to cpburnz/python-pathspec#50

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems fragile to test the exact error message produced by pathspec. Maybe we should check only for the "Could not parse" bit from black's own code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense to me! Especially if we'd be upgrading the dependency quietly.

Copy link
Collaborator

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Thank you!

)
assert result.exit_code == 1
assert result.stderr_bytes is not None
assert b"Invalid git pattern: '!\\n'" in result.stderr_bytes
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems fragile to test the exact error message produced by pathspec. Maybe we should check only for the "Could not parse" bit from black's own code?

assert result.stderr_bytes is not None

gitignore = path / ".gitignore"
assert f"Could not parse {gitignore}" in result.stderr_bytes.decode()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here - it only checks for the presence of black's own error message

@ichard26 ichard26 self-requested a review August 7, 2021 17:35
Copy link
Collaborator

@ichard26 ichard26 left a comment

Choose a reason for hiding this comment

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

This seems fine to me, but I will note that code wasn't immediately clear and I was quite scared about "if gitignore is None: sys.exit(1)" bit since I thought it would also exit on cases where a gitignore file didn't exist.

Nevertheless, thank you!

@nipunn1313
Copy link
Contributor Author

This seems fine to me, but I will note that code wasn't immediately clear and I was quite scared about "if gitignore is None: sys.exit(1)" bit since I thought it would also exit on cases where a gitignore file didn't exist.

Nevertheless, thank you!

Good point. One idea could be to lift the except GitWildMatchPatternError to the callsite - instead of converting it to a None within the function. That might be more obvious at the callsite. Thoughts?

Another option might be to move the exit(1) downward into the helper method - but that seems a little too magical for my taste (would make it hard to unit test for example).

Copy link
Collaborator

@ichard26 ichard26 left a comment

Choose a reason for hiding this comment

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

Actually this doesn't go far enough, it didn't occur to me before but we actually support nested gitignores these days, and uhm, this PR doesn't fully fix the ungraceful crash when a nested gitignore is invalid.

tree -a test
test
├── a
│   ├── a.py
│   └── .gitignore
└── a.py

1 directory, 3 files

~/programming/oss/black on gitignore [$?] via v3.8.5 (bm-venv) dropblackcache -y ; black test
no caching was found!
Could not parse test/a/.gitignore: Invalid git pattern: '!\n'
Traceback (most recent call last):
  File "/home/ichard26/programming/oss/black/bm-venv/bin/black", line 33, in <module>
    sys.exit(load_entry_point('black', 'console_scripts', 'black')())
  File "/home/ichard26/programming/oss/black/src/black/__init__.py", line 1281, in patched_main
    main()
  File "/home/ichard26/programming/oss/black/bm-venv/lib/python3.8/site-packages/click/core.py", line 1137, in __call__
    return self.main(*args, **kwargs)
  File "/home/ichard26/programming/oss/black/bm-venv/lib/python3.8/site-packages/click/core.py", line 1062, in main
    rv = self.invoke(ctx)
  File "/home/ichard26/programming/oss/black/bm-venv/lib/python3.8/site-packages/click/core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/home/ichard26/programming/oss/black/bm-venv/lib/python3.8/site-packages/click/core.py", line 763, in invoke
    return __callback(*args, **kwargs)
  File "/home/ichard26/programming/oss/black/bm-venv/lib/python3.8/site-packages/click/decorators.py", line 26, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/home/ichard26/programming/oss/black/src/black/__init__.py", line 431, in main
    sources = get_sources(
  File "/home/ichard26/programming/oss/black/src/black/__init__.py", line 536, in get_sources
    sources.update(
  File "/home/ichard26/programming/oss/black/src/black/files.py", line 227, in gen_python_files
    gitignore + get_gitignore(child) if gitignore is not None else None,
TypeError: unsupported operand type(s) for +: 'PathSpec' and 'NoneType'

Sorry for not realizing this earlier!

@nipunn1313
Copy link
Contributor Author

ooh thanks for catching! Will cover and add test

@nipunn1313
Copy link
Contributor Author

Updated! Great catch. I went ahead and caught the exception one method higher so that it covers both callsites. Added a test for the nested case as well (it failed before, now it passes).

Copy link
Collaborator

@ichard26 ichard26 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 quick update!

@ichard26 ichard26 merged commit 104aec5 into psf:main Aug 20, 2021
@ichard26
Copy link
Collaborator

Thank you so much for your contribution! This project is only possible by contributions like these 🖤. You're awesome, @nipunn1313. It's great to see the project get easier / more friendly to use :)

Congrats on your first PR here! If you have any feedback on experience contributing please let me know via #2238.

@nipunn1313 nipunn1313 deleted the gitignore branch October 15, 2021 04:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Present a more user-friendly error if .gitgignore
3 participants