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

Appearance settings

Add pre-commit-assert-changelog to all-repos#629

Closed
SamStudio8 wants to merge 1 commit into
pre-commit:masterpre-commit/pre-commit.com:masterfrom
SamStudio8:masterSamStudio8/pre-commit.com:masterCopy head branch name to clipboard
Closed

Add pre-commit-assert-changelog to all-repos#629
SamStudio8 wants to merge 1 commit into
pre-commit:masterpre-commit/pre-commit.com:masterfrom
SamStudio8:masterSamStudio8/pre-commit.com:masterCopy head branch name to clipboard

Conversation

@SamStudio8

Copy link
Copy Markdown

No description provided.

Comment thread all-repos.yaml
- https://github.com/JohnnyMorganz/StyLua
- https://github.com/shssoichiro/oxipng
- https://github.com/datarootsio/databooks
- https://github.com/SamStudio8/pre-commit-assert-changelog

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • there's no readme
  • there's no license
  • you've got a few security bugs waiting to happen (shell=True, sys.path.append('.'))
  • executables are typically kebab-case not snake_case
  • your project should not contain pre-commit- as it is not affiliated with pre-commit

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thank you for taking a look!

  • README and license added
  • Removed shell=True and use sys.path.append("") as per expectation for when script dir is not available
  • Executable is now assert-changelog
  • README clarifies this is a pre-commit hook and not a pre-commit project but I am happy to change the repo -- I just followed what seemed to be the general consensus on github

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

pre-commit doesn't run with unstaged changes so I don't think this hook will work at all

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The behaviour I'm seeing is pre-commit emits a warning on unstaged changes and stashes them. The list of unstaged files is inferred by assert-changelog (https://github.com/SamStudio8/pre-commit-assert-changelog/blob/main/assert_changelog/main.py#L108). It demonstrably works for me but happy to admit this is a very alpha project that perhaps should not be in your list.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

--cached is an alias for --staged so I suspect your tool is not working

@SamStudio8 SamStudio8 closed this Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.