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.

Sign up for GitHub

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

Jump to bottom

Support version specifiers in GH action #3265

Merged
merged 4 commits into from Sep 23, 2022
+25 −6
Merged

Support version specifiers in GH action #3265

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
feada3f
Support version specifiers in GH action
Jackenmen Sep 8, 2022
e7417d3
Add changelog entry
Jackenmen Sep 8, 2022
63fec34
Document the new behavior
Jackenmen Sep 11, 2022
e537d41
Huh, how'd that get in there?
Jackenmen Sep 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGES.md
View file
Open in desktop
Expand Up @@ -42,6 +42,9 @@

<!-- For example, Docker, GitHub Actions, pre-commit, editors -->

- Update GitHub action to support use of version specifiers (i.e. `<23`) for Black
version (#3265)
Jackenmen marked this conversation as resolved.
Show resolved Hide resolved

### Documentation

<!-- Major changes to documentation and policies. Small docs changes
Expand Down
7 changes: 4 additions & 3 deletions action/main.py
View file
Open in desktop
Expand Up @@ -14,9 +14,10 @@

run([sys.executable, "-m", "venv", str(ENV_PATH)], check=True)

req = "black[colorama]"
if VERSION:
req += f"=={VERSION}"
version_specifier = VERSION
if VERSION and VERSION[0] in "0123456789":
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if VERSION and VERSION[0] in "0123456789":
if VERSION and VERSION[0].isdigit():

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah damn, I didn't notice this comment before I made mine 😄 I can change it, though I would like to note that isdecimal is less wide check than isdigit so it might be better choice if we're switching it.

Copy link
Contributor Author

@Jackenmen Jackenmen Sep 8, 2022 •
edited

Choose a reason for hiding this comment

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

I could do VERSION[0].isdecimal() here though that's rather wide due to it supporting all Unicode characters in the Unicode General Category “Nd”. Personally, I tend to avoid isdecimal/isdigit/isnumeric methods because they evaluate to True for more than I actually want but I can change this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess it's fine either way, since we only care about recognising version specifier characters, which won't be decimal or digits. But no biggie.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do packaging tools normalize non-ascii digits? If so, I'd prefer isdecimal() otherwise I'd prefer being stricter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pip doesn't handle Unicode numbers:

❯ pip install 'black==२२.१'
ERROR: Could not find a version that satisfies the requirement black==२२.१ (from versions: 18.3a0, 18.3a1, 18.3a2, 18.3a3, 18.3a4, 18.4a0, 18.4a1, 18.4a2, 18.4a3, 18.4a4, 18.5b0, 18.5b1, 18.6b0, 18.6b1, 18.6b2, 18.6b3, 18.6b4, 18.9b0, 19.3b0, 19.10b0, 20.8b0, 20.8b1, 21.4b0, 21.4b1, 21.4b2, 21.5b0, 21.5b1, 21.5b2, 21.6b0, 21.7b0, 21.8b0, 21.9b0, 21.10b0, 21.11b0, 21.11b1, 21.12b0, 22.1.0, 22.3.0, 22.6.0, 22.8.0)
ERROR: No matching distribution found for black==२२.१

So in the end, it shouldn't matter whether we use isdecimal() or the in check as it will fail anyway.

version_specifier = f"=={VERSION}"
req = f"black[colorama]{version_specifier}"
pip_proc = run(
[str(ENV_BIN / "python"), "-m", "pip", "install", req],
stdout=PIPE,
Expand Down