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

added warning if mutable rev is used #1715

Open
wants to merge 1 commit into
base: master
from

Conversation

@paulhfischer
Copy link
Contributor

@paulhfischer paulhfischer commented Nov 25, 2020

resolves #974

  • only added a warning instead of raising an error because this might break multiple projects otherwise
  • not sure if other mutable revs should also cause a warning, couldn't think of a way to detect them in general …
  • the check could also be included in the MigrateShaToRev-class, but i think it's better to keep them separated
  • test only checks for HEAD, this is sufficient imo, but i can also change it so it tests for all mutable revs defined in {'HEAD', 'stable', 'master'}
@paulhfischer paulhfischer force-pushed the paulhfischer:warn_if_mutable_rev_is_used branch from 2773384 to 1a2b4a0 Nov 25, 2020
pre_commit/clientlib.py Show resolved Hide resolved
def check(self, dct: Dict[str, Any]) -> None:
if any(
mutable in dct.get(self.key, '')
for mutable in {'HEAD', 'stable', 'master'}

This comment has been minimized.

@asottile

asottile Nov 26, 2020
Member

I'm not sure this will catch all the cases, notably it's missing '', 'main' at the very least

I wonder if there's an easier way to catch these -- maybe match anything that doesn't look like a version?

This comment has been minimized.

@paulhfischer

paulhfischer Nov 26, 2020
Author Contributor

alternative idea (not added yet, wanted to hear your feedback first):

rev = dct.get(self.key, "")

if (
    "." not in rev and (  # not a version
        len(rev) != 40 or  # full sha
        len(rev) != 7 or  # short sha
        len(rev) != 8 or  # gitlab lists 8 char long sha in the commit-list
    ) or
    not re.match(r"^[a-zA-Z0-9]+$", rev)  # contains other chars (e.g. "_", "-", …)
)
# print warning
pre_commit/clientlib.py Outdated Show resolved Hide resolved
pre_commit/clientlib.py Outdated Show resolved Hide resolved
pre_commit/clientlib.py Outdated Show resolved Hide resolved
@paulhfischer paulhfischer force-pushed the paulhfischer:warn_if_mutable_rev_is_used branch from 1a2b4a0 to caa8451 Nov 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.