-
-
Notifications
You must be signed in to change notification settings - Fork 710
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
forbid-new-submodules fixes #619
Conversation
def main(argv: Optional[Sequence[str]] = None) -> int: | ||
# `argv` is ignored, pre-commit will send us a list of files that we | ||
# don't care about | ||
def main(argv: Sequence[str] = sys.argv[1:]) -> int: |
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.
this sets a mutable default value which does not work as you would expect in python -- please don't change this, the original argument handling was correct
>>> def f(x=[]):
... x.append('wat')
... print(x)
...
>>> f(['hi'])
['hi', 'wat']
>>> f(['hi'])
['hi', 'wat']
>>> f()
['wat']
>>> f()
['wat', 'wat']
>>> f()
['wat', 'wat', 'wat']
>>> f()
['wat', 'wat', 'wat', 'wat']
>>> f()
['wat', 'wat', 'wat', 'wat', 'wat']
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.
Yep, I'm aware of the behavior of mutable defaults in python. In this case it's not a problem because the function will always be called only once and without arguments by setuptools machinery. However, I agree that using it may be fragile, for example, in tests.
if ( | ||
'PRE_COMMIT_FROM_REF' in os.environ and | ||
'PRE_COMMIT_TO_REF' in os.environ | ||
): | ||
diff_args: Sequence[str] = ( | ||
'--merge-base', | ||
os.environ['PRE_COMMIT_FROM_REF'], | ||
os.environ['PRE_COMMIT_TO_REF'], | ||
) | ||
else: | ||
diff_args = ('--staged',) | ||
added_diff = cmd_output( | ||
'git', 'diff', '--staged', '--diff-filter=A', '--raw', | ||
'git', 'diff', '--diff-filter=A', '--raw', *diff_args, '--', *argv, | ||
) |
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.
this shouldn't be needed -- use the arguments that are passed from pre-commit directly (you'll want to set up an argparse.ArgumentParser
-- see how the rest of the tools do this)
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.
Initially I thought that too and wanted to rewrite it using git status
instead of git diff
to query mode. However, the hook is called "forbid new submodules" which apparently means that bumping somehow already existent submodules should be allowed. That's why we need --diff-filter=A
and we need to query PRE_COMMIT_*_REF
variables in order to ensure that no submodules have been added in any commits being validated, not only the last one.
tests/forbid_new_submodules_test.py
Outdated
finally: | ||
del os.environ['PRE_COMMIT_FROM_REF'] | ||
del os.environ['PRE_COMMIT_TO_REF'] |
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.
use mock.patch.dict
, though I think this code will be deleted anyway
1886f67
to
fcf2962
Compare
Finally managed to take a look into tests failure on Windows and fix it. Please, take a look. |
if not args.filenames: | ||
return 0 |
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.
pre-commit will never call this with zero arguments
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.
Yeah, I've added it because of git diff
or git status
behavior difference with one or more path arguments (check them) vs. zero (check all, not none). But I agree that this is not needed.
'PRE_COMMIT_FROM_REF' in os.environ and | ||
'PRE_COMMIT_TO_REF' in os.environ | ||
): | ||
diff_args: Sequence[str] = ( |
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.
Tuple[str, ...]
tests/forbid_new_submodules_test.py
Outdated
REV_PARSE_HEAD = ('git', 'rev-parse', 'HEAD') | ||
FROM = subprocess.check_output(REV_PARSE_HEAD).decode().strip() |
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.
why are you capitalizing local variables
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.
Fixed FROM
and TO
, preserved REV_PARSE_HEAD
because it's a constant. If you insist, I can put it to the top of the module (since it only needed in this function I've put it there).
tests fail on git 2.25.1 |
…s committed (without any other file); support --from-ref and --to-ref; fixes pre-commit#609
Fixed. |
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.
Fixes for #609.