-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Validate 'Created:' dates on CI #1809
Conversation
See also #1805 |
date = line.removeprefix("Created:").strip() | ||
try: | ||
# Validate | ||
dt.strptime(date, "%d-%b-%Y") |
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.
The "%d-%b-%Y"
format depends on the current locale; it might be different across machines.
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.
Ah right, your suggestion at #1805 (comment) for something like %Y-%m-%d
(2021-02-08
) would help with this.
Great! That saves me a lot of work :) Rerunning on the top of that branch, everything validates except for: $ make lint
pre-commit --version > /dev/null || python3 -m pip install pre-commit
pre-commit run --all-files
rst ``code`` is two backticks............................................Passed
rst ``inline code`` next to normal text..................................Passed
Lint Created: dates......................................................Failed
- hook id: validate-created
- exit code: 1
pep-0102.txt:11:8: unconverted data remains: (edited down on 9-Jan-2002 to become PEP 102)
make: *** [lint] Error 1 From: Line 11 in 604b513
Should this line be changed to pass the current validation, or the validation changed to accept this? |
75bec84
to
1c70eed
Compare
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.
Hey, happened to see this PR and wanted to make sure it wasn't affected by pre-commit/pygrep-hooks#51 . Its not, but I did notice something else:
- Because
stages
isn't set (i.e. to["manual"]
), the validation won't just run on CIs as indicated by the title and description, but rather whenever the user runs pre-commit, either as a pre-commit hook (if they install them),pre-commit run --all-files
, or the non-standardmake lint
. - If it isn't desired (and there are several problems if it is), the easiest way to fix this is just add
stages: ["manual"]
to the hook config and add the following here in your Github workflow, which will run both the default hooks and the manual one as well:- uses: pre-commit/[email protected] with: extra_args: --all-files --hook-stage manual
- If this is the intended behavior, the PR title should presumably be updated. However, be aware that running this hook by default will likely cause the pre-commit run to fail the run if users are on Windows (no
python3
, python is just calledpython
there) or don't have Python 3.9+ as the default on their system/current env (quite likely), so I would recommend the former.
Also, as you might already be aware, language: script
, entry: ./scripts/validate-created.py
and a shebang and executable mode on the script is the nominal way to call scripts like this with pre-commit, but AFAIK there's isn't much of a practical difference doing it like this instead.
1c70eed
to
c66d81c
Compare
Thanks for checking! Python 3.9: I've pushed an update which allows #1809 (comment) to validate and also drops Windows: good point, although we have a possibly related issue to decide on about locale: #1809 (comment). CI + local: my intention was to run both on CI and locally, and it currently does so. But if we decide only to run this check on Linux then your suggestions about running on only CI may help. (Possibly hoisting the check out of pre-commit.) |
Sure! I did notice that the older PEPs with the Python 3.9: Sounds good! Locale: IMO, I would strongly support a standard, locale-independent format that's unambiguous and easy to parse by both programs and programmers, i.e. ISO 8601/RFC 3339 (`%Y-%m-%d/YYYY-MM-DD). Particularly in this case, it seems a bit unnecessary to require anyone running the script to set their locale to English or get incorrect results. But that's just my personal opinion, its ultimately up to you all and I'm happy to help with whatever you decide. CI: Yeah, ideal to run them on both; I have some ideas to resolve the cross-platform compat issue without having to make it CI-only. If you do that, I'd suggest just confining it to the manual hook-stage; less work and less bespoke code, plus nicer UX. Windows: Yup, that comes back to the perennial issue that there's no reliable cross-platform way to invoke Python that works across Windows, *nix, venvs and conda; I recall reading yet another thread on that on Python-dev lately. In this case, we know a Python environment must be accessible (because Pre-Commit itself is running in it), but we don't know whether it is exposed at Maybe we can ask Anthony if he would accept a PR adding a However, in this case you should be able to get away with a ^(.*\n){1,20}Created: ([0-2][0-9]|(3[01]))-(Jan|Feb|Mar|Apr|May|Jun|Jul|Aug|Sep|Oct|Nov|Dec)-(199[0-9]|20[0-9][0-9])( \(|\n) Here's the equivalent in YYYY-MM-DD: ^(.*\n){1,20}Created: ((199[0-9]|20[0-9][0-9])-(0[0-9]|1[0-2])-[0-2][0-9]|(3[01]))( \(|\n) |
I like the current format. I find it unambiguous and easier to read than
2021-03-12.--
--Guido (mobile)
|
Yeah, it is unambiguous and easy enough read since it includes The practical issue is that |
Right, so we keep the existing format. @CAM-Gerlach I agree, makes sense to unhitch this at least the local pre-commit, maybe even move it outside pre-commit to run on CI. Sounds like you have some solid ideas and I'm a little swamped at the moment, would you like to come up with something? Feel free to take anything from this PR or start fresh, and either update my fork branch or just submit a new PR. Thank you! |
any tools reading (or writing) it will break if they don't happen to be
run in an English local
IMO the PEP format is pretty specialized and requires specialized tools, so
it behooves the authors of those tools not to depend on the user's locale.
|
Sorry for the delay; it was a busy week with other things. @hugovk Sure, I ended up just starting fresh with creating a simple pygrep hook as proposed above, as well as addressing a few related issues. I'll open a PR momentarily, thanks. @gvanrossum Right; as mentioned the solutions above and in the followup PR avoids locale dependence completely and doesn't require changing the existing format. |
Please see #1886 instead. |
Fixes #1803.
Also added to the
make lint
.As we can see, 87 currently don't validate:
I'll fix them and update the PR.