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

Upgrade lint infra & add a few more conservative built-in checks #2286

Merged
merged 5 commits into from
Feb 1, 2022
Merged

Upgrade lint infra & add a few more conservative built-in checks #2286

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2a5dbf2
Add and clarify documentation/UI text for pre-commit and actions
CAM-Gerlach Jan 27, 2022
cce31b8
Upgrade pre-commit hook and GH action versions and boilerplate
CAM-Gerlach Jan 27, 2022
28d2393
PEP 465: Move ambigiously-named file out of root; update codeowners
CAM-Gerlach Jan 27, 2022
b884f77
Add a few additional built-in pre-commit checks and fix issues detected
CAM-Gerlach Jan 28, 2022
d67d3b8
Update .pre-commit-config.yaml
JelleZijlstra Feb 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions .github/CODEOWNERS
View file
Open in desktop
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* @python/pep-editors

# PEP infrastructure
.github/workflows/ @AA-Turner
.github/workflows/ @AA-Turner @CAM-Gerlach
Makefile @AA-Turner
requirements.txt @AA-Turner
.pre-commit-config.yml @CAM-Gerlach
Expand Down Expand Up @@ -38,6 +38,7 @@ pep-0009.txt @warsaw
pep-0010.txt @warsaw
pep-0011.txt @brettcannon
pep-0012.rst @brettcannon @warsaw
pep-0012/ @brettcannon
Copy link
Member

Choose a reason for hiding this comment

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

Not going to comment on each of these individually -- I don't think any of these are required, though? If a PEP author cares enough about ancilliary files (as I believe you intend to do with your metadata one), he can add himself here manually.

I think I'd also prefer these were in an "ancilliary files" section instead of inline if we did do this, but as above I'd prefer we didn't

Copy link
Member Author

Choose a reason for hiding this comment

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

Not going to comment on each of these individually -- I don't think any of these are required, though? If a PEP author cares enough about ancilliary files (as I believe you intend to do with your metadata one), he can add himself [sic] here manually.

The only changes (other than adding myself as co-maintainer of the GitHub actions subdir, since I maintain the lint action, and have background experience in this area) were fixing two minor syntax issues, updating it to reflect the move of the confusingly-named/located scan_ops.py file to follow PEP 1 like all the others (and make clear its not infra), and adding Brett as owner of the PEP 12 template, as creator and PEP co-owner (which appeared to be a simple oversight).

Other than that, all of the other subdirs and various ancillary files were assigned to their PEP author/sponsor—and per the intent, if not the explict letter of PEP 1, PEP authors/sponsors should be responsible for and notified about the files specific to their PEPs (as is their designated role per PEP 1), and that should be clear to others from the GitHub UI when one is modified, unless there is good cause in a particular circumstance to explicitly opt out.

@brettcannon is this okay with you?

I think I'd also prefer these were in an "ancilliary files" section instead of inline if we did do this, but as above I'd prefer we didn't

Changing the current organization scheme is a much bigger change than this PR and is out of scope here, which just makes a few tweaks to fix a couple apparent oversights of auxiliary files getting out of sync with PEPs—a problem which not keeping the two together would tend to exacerbate.

he can add himself

Just FYI, I can't speak for BrE, but using "he" to refer to persons of unspecified gender (not all PEP authors are men) is considered pretty objectionable (and rather archaic/uncommon) in modern, idiomatic AmE and on this repo; see #855 , #2214 , #2258

# pep-0013.rst is owned by the entire core team.
# ...
pep-0020.txt @tim-one
Expand Down Expand Up @@ -319,6 +320,7 @@ pep-0462.txt @ncoghlan
# pep-0463.txt
pep-0464.txt @dstufft
pep-0465.txt @njsmith
pep-0465/ @njsmith
pep-0466.txt @ncoghlan
pep-0467.txt @ncoghlan @ethanfurman
pep-0468.txt @ericsnowcurrently
Expand Down Expand Up @@ -478,7 +480,7 @@ pep-0603-hamt_vs_dict.png @1st1
pep-0605.rst @zooba @ncoghlan
pep-0605-example-release-calendar.png @zooba @ncoghlan
pep-0605-overlapping-support-matrix.png @zooba @ncoghlan
/pep-0605/ @zooba @ncoghlan
pep-0605/ @zooba @ncoghlan
pep-0606.rst @vstinner
pep-0607.rst @ambv @zooba @ncoghlan
pep-0608.rst @vstinner
Expand Down Expand Up @@ -536,7 +538,7 @@ pep-0659.rst @markshannon
pep-0660.rst @pfmoore
pep-0661.rst @taleinat
pep-0662.rst @brettcannon
pep-0662/pep-0662-editables.json @brettcannon
pep-0662/ @brettcannon
pep-0663.txt @ethanfurman
pep-0664.rst @pablogsal
pep-0665.rst @brettcannon
Expand Down
6 changes: 4 additions & 2 deletions .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
name: Build
name: Docutils Build
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this churn is warranted -- the workflow will be retired soon (TM), and all that need to care about which is Docutils-only and which is Sphinx already know

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think its really fair to dismiss as mere "churn" a change that clarifies names that are actively confusing, at least to me as a PEP editor (and one quite familiar with the build system) and likely to others as well. In particular here, there's no obvious indication as to whether the Build check refers to the Docutils build or, as might logically be assumed, the Sphinx build (given it is the latter that is invoked by the file named build.py). I only figured out what they actually referred to after digging through the various files, and I still have to remind myself sometimes.

I'm not sure how one could argue that being clearer and more descriptive is a bad thing, so given it isn't retired yet, while I welcome your suggestions on revised or alternative names, if you feel it isn't worthwhile to spend time on it the most logical course of action would be to just use it for now, rather than both of us wasting time debating (or reverting) it.


on: [push, pull_request, workflow_dispatch]

jobs:
build:
name: Build with Docutils
runs-on: ubuntu-latest

steps:
- uses: actions/checkout@v2
- name: Check out repo
Copy link
Member

Choose a reason for hiding this comment

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

Style -- I never name the checkout action step

Copy link
Member Author

@CAM-Gerlach CAM-Gerlach Jan 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

...Except you did here on the other workflow in this repo, complete with an emoji even 😝

I added it there for consistency with the others, which all had names and had name as their first key. Its not terribly important, but neither does it do any harm, and it would be a non-zero amount of work (and rebasing) to revert it,

uses: actions/checkout@v2

- name: Set up Python
uses: actions/setup-python@v2
Expand Down
9 changes: 5 additions & 4 deletions .github/workflows/deploy-gh-pages.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
name: Deploy to GitHub Pages
name: Sphinx Build
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer not to do this now. Fixing up everything on preview build + etc is on my to do list, but I want outcome of 676 & backend provider done first. We may entirely delete this if RTD is used, for example.

Copy link
Member Author

@CAM-Gerlach CAM-Gerlach Jan 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Again, I don't think we should let the perfect be the enemy of the good and actively reject a change that clarifies something causing active confusion right now (potentially not only for PEP editors, but authors as well), just because we might change it at some point in the future. At least in the context of running on each PR, which is likely how 95% of people are going to actually see it (especially those besides you), calling the whole workflow "Deploy to GitHub pages" when that is actively confusing as that is the one thing it doesn't do (PEP authors might assume, for instance, that it actually deploys their PEPs live somewhere), and also not being clear what build system is involved (one might assume the same one in the Build job because of the very confusing names).

I find it difficult to argue that clarifying this, even imperfectly, is not an improvement over the status quo, and if you feel its not worth spending more effort on, the least-effort course of action for all involved is going with the above for now.


on: [push, pull_request, workflow_dispatch]

jobs:
deploy-to-pages:
name: Build & deploy to GitHub Pages
Copy link
Member

Choose a reason for hiding this comment

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

Does this need a name?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is clearer, cleaner and more descriptive in the UI, so I don't see how it is a net harm. If you'd like to suggest a different name, feel free of course.

runs-on: ubuntu-latest

steps:
Expand All @@ -15,13 +16,13 @@ jobs:
- name: 🐍 Set up Python 3
uses: actions/setup-python@v2
with:
python-version: 3
python-version: '3.x'
Copy link
Member

Choose a reason for hiding this comment

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

"3" is cleaner and works

Copy link
Member

Choose a reason for hiding this comment

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

I recommend following GitHub's docs and use '3.x', they tend to remove undocumented things with no notice.

For example, pypy3 is being removed and you need to use something like pypy-3.8 instead: actions/setup-python#244.

Copy link
Member

Choose a reason for hiding this comment

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

The docs you linked to refer to SemVer ranges -- whilst not linked, I believe it means semver as in the npm package, given it is used within the action (https://github.com/actions/setup-python/blob/156361d073e2d704ec0bfe646b4500cdc0547324/src/find-python.ts#L5).

semver the package documents that the "x" is optional. (see https://github.com/npm/node-semver#x-ranges-12x-1x-12-)

A partial version range is treated as an X-Range, so the special character is in fact optional.

Sorry, appreciate I didn't mention this originally.

A

Copy link
Member Author

Choose a reason for hiding this comment

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

Taking a step back, I'm having trouble understanding what justifies spending a bunch of our time and churn researching, debating and reverting this instead of simply using what is in GitHub's official documentation and leaving it at that, rather than taking a risk just to fulfill one person's personal stylistic preference on a single key value. If I'd known it would become a huge deal I wouldn't have dreamed of touching it, and I certainly won't make that mistake again—and I should have known better, from past experience.

cache: "pip"

- name: 👷‍ Install dependencies
run: |
python -m pip install --upgrade pip
python -m pip install -r requirements.txt
python -m pip install --upgrade -r requirements.txt
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand this one -- why do we need -U. (As with you I'm more used to conda, so this might just be something on pip's install logic I am not aware of.)

Copy link
Member Author

Choose a reason for hiding this comment

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

The issue here is we're not installing in to a fresh venv, but rather the existing Python, so some deps could already exist either from being system-installed, and/or injected by default into the env. Including --upgrade ensures at least first-level deps get upgraded, if possible.

As a sidenote, for repos of Python packages, rather than docs, websites, etc like this one, I keep everything clean by using venvs/conda envs. To note, even when creating a fresh venv with python -m venv, cached copies of some existing deps, including pip, setuptools and pkg_resources get injected into it, instead of downloading fresh copies of conda create. So its a good idea to add setuptools and wheel to the first line to ensure they are available and up to date for non-PEP 517 installs, though I didn't add them here to avoid another big debate.


- name: 🔧 Build PEPs
run: make pages -j$(nproc)
Expand All @@ -33,7 +34,7 @@ jobs:
- name: 🚀 Deploy to GitHub pages
# This allows CI to build branches for testing
if: github.ref == 'refs/heads/main'
uses: JamesIves/github-pages-deploy-action@4.1.1
uses: JamesIves/github-pages-deploy-action@v4.2.2
with:
branch: gh-pages # The branch to deploy to.
folder: build # Synchronise with build.py -> build_directory
Expand Down
15 changes: 12 additions & 3 deletions .github/workflows/lint.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,17 @@ on: [push, pull_request, workflow_dispatch]

jobs:
pre-commit:
name: Run pre-commit
runs-on: ubuntu-latest

steps:
- uses: actions/checkout@v2
- uses: actions/setup-python@v2
- uses: pre-commit/[email protected]
- name: Check out repo
Comment on lines +7 to +11
Copy link
Member

Choose a reason for hiding this comment

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

as above on we don't need to give everything a descriptive name

Copy link
Member Author

@CAM-Gerlach CAM-Gerlach Jan 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe I missed it somewhere (please do point it out if so), but I can't seem to find where you've explained why it is actively harmful to do so, when it adds descriptiveness and clarity (as well as looks more polished) in the GitHub UI. The only thing I noticed you mention was that simply that you personally don't add it in your own work, which I don't see how has any bearing on blocking me from adding it here in mine, particularly when there are concrete (clarity, descriptiveness and clean UI) reasons to do otherwise and on a workflow I'm responsible for maintaining.

I don't personally care for the use of emojis in step names (and more practically, they don't show up at all in my editor), but it would be equally inappropriate for me to propose or request removing them from the workflow you added due that personal preference.

uses: actions/checkout@v2

- name: Set up Python 3
uses: actions/setup-python@v2
with:
python-version: '3.x'
Copy link
Member

Choose a reason for hiding this comment

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

Keep this as "3" for consistency with the other one

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, but I would prefer to follow GitHub's official documentation and avoid any risk of the workflow breaking simply to conform to someone else's personal stylistic preference. See that discussion for more.


- name: Run pre-commit hooks
uses: pre-commit/[email protected]
66 changes: 55 additions & 11 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,33 +1,77 @@
# See https://pre-commit.com for more information
# See https://pre-commit.com/hooks.html for more hooks
Comment on lines +1 to +2
Copy link
Member

Choose a reason for hiding this comment

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

These can be removed

Copy link
Member Author

Choose a reason for hiding this comment

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

I retained this it is helpful to explicitly link to the documentation of the syntax (first line) and the hooks used (second line), so that those coming across the file can quickly navigate to the canonical source explaining these two aspects.


minimum_pre_commit_version: '2.8.2'
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes; it is a recommended best practice that ensures users (and the CI) has a sufficiently up to date version of pre-commit that supports the syntax and semantics used by the config and hooks used, as well as doesn't run into any edge-case bugs and compat issues from older versions. The most recent widely used major addition in this vein was types_or in 2.8.x, IIRC. Older versions also use a different config format and don't support other hooks/options used here.


default_language_version:
python: python3

default_stages: [commit]
Comment on lines +6 to +9
Copy link
Member

Choose a reason for hiding this comment

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

Why are these new defaults needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

The default_language_version to ensure Python 3 is used on hooks that don't specify it, but I think its redundant nowadays? Doesn't hurt to have it, but probably doesn't hurt to remove it either?

The default_stages ensures that pre-commit run --hook-stage manual only runs the manual hooks (i,e. codespell, and any others we may add); otherwise, it would run all hooks and there wouldn't be a way to run only manual hooks, which often comes in handy.



repos:
# General file checks and fixers
- repo: https://github.com/pre-commit/pre-commit-hooks
rev: v3.4.0
rev: v4.1.0
hooks:
- id: mixed-line-ending
name: Normalize mixed line endings
name: "Normalize mixed line endings"
args: [--fix=lf]
- id: fix-byte-order-marker
name: "Remove Unicode BOM"
- id: file-contents-sorter
name: "Sort codespell ignore list"
files: '.codespell/ignore-words.txt'

- id: check-case-conflict
name: "Check for case conflicts"
- id: check-merge-conflict
name: "Check for merge conflict markers"
- id: check-executables-have-shebangs
name: "Check that executables have shebangs"
- id: check-shebang-scripts-are-executable
name: "Check that shebangs are executable"

- id: check-vcs-permalinks
name: "Check that VCS links are permalinks"
CAM-Gerlach marked this conversation as resolved.
Show resolved Hide resolved

- id: check-ast
name: "Check Python AST"
- id: check-json
name: "Check JSON"
- id: check-toml
name: "Check TOML"
CAM-Gerlach marked this conversation as resolved.
Show resolved Hide resolved
- id: check-yaml
name: "Check YAML"
Comment on lines +20 to +45
Copy link
Member

Choose a reason for hiding this comment

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

What justifies all these new checks? The PEPs repo is fundamentally a text repo -- I don't want to see us loosing the wood for the trees. (Personally I'd also e.g. remove the line ending check as .gitattributes handles that, but does more harm in creating churn to remove it at this point).

Copy link
Member Author

Choose a reason for hiding this comment

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

I took pains to be pretty conservative and only enabled a limited number of built-in checks after careful investigation and testing that were not just style-related and had an articulatable benefit for this repo, not enabling any that required fixing a few existing files in the repo, none that required dealing with any existing false positives throughout all past PEPs (except for the new PEP link check you specifically requested), and not adding any code checkers, linters or formatters (beyond simply verifying valid AST).

  • The BOM check avoids a spurious Unicode BOM injected into text files, as older versions of Notepad do by default and some other text editors can do if set incorrectly.
  • The file contents sorter keeps the codespell ignore list sorted and easy to scan. It only runs if that file is modified.
  • The case conflict checks for duplicate files and directories that differ only by case (on Unix and Linux), which will cause problems for Git and users on non-case-sensitive filesystems (Windows and most macOS). Not common, but difficult to fix once it gets committed.
  • The shebang checks detect instances of the executable bit and the shebang not matching, which is pretty easy to forget especially for users on non-POSIX systems and is fairly easy to fix but hard for reviewers to spot; this I've noticed on multiple PRs just in the past couple weeks (as you know).
  • The VCS permalinks check we've discussed; it detects instances of non-perma links to specific lines of files that are likely to quickly become stale; we could extend it to check all VCS links (but that might cause a few false positives, since occasionally there could be reasons for wanting to link the "current" version rather than a specific one).
  • The file validation checkers only run if a file with that format is added or modified and contains a critical error (typo, mistake, etc) that would prevent it from being parsed at all, which is certainly quite possible if users are not being careful. In the unlikely event that a deliberately invalid file might be added (so far, no cases of such), it could simply be added to exclude for that check and the check won't run at all on it; so far none of the files added are false positives for this.

The extra checks cost users nothing unless they explicitly choose to install our pre-commit hooks and edit a file the checks validate, and even in that case only add a couple extra seconds total once per commit (and on the CI lint run, which is still the fasted CI of the three by several times over). False positives seem rather unlikely, given none of them I ended up adding produced any on the repo's entire history (I rejected those that did).


# RST checks
- repo: https://github.com/pre-commit/pygrep-hooks
rev: v1.8.0
rev: v1.9.0
hooks:
- id: rst-backticks
name: "Check RST: No single backticks"
- id: rst-inline-touching-normal
name: "Check RST: No backticks touching text"
files: '^pep-\d+\.txt|\.rst$'
types: [text]
- id: rst-directive-colons
name: "Check RST: 2 colons after directives"
files: '^pep-\d+\.txt|\.rst$'
types: [text]

# Manual codespell check
Copy link
Member

Choose a reason for hiding this comment

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

If we're going to change all the "name" fields from here down, please can we say "Validate the PEP 'X' field"

Copy link
Member Author

Choose a reason for hiding this comment

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

These names need to be as short as practical while still conveying the key information, since they get printed to the often very limited width of the user's terminal in quick succession, and by general convention, are framed as titles/fragments rather than complete sentences. Therefore, including extraneous and redundant words like "the" is not really necessary here.

- repo: https://github.com/codespell-project/codespell
rev: v2.1.0
hooks:
- id: codespell
name: Check for common misspellings in text files
name: "Check for common misspellings in text files"
stages: [manual]

# Local checks for PEP headers and more
- repo: local
hooks:
- id: check-required-fields
name: "Check all PEPs have required fields"
name: "Check PEPs have all required fields"
language: pygrep
entry: '(?-m:^PEP:(?=[\s\S]*\nTitle:)(?=[\s\S]*\nAuthor:)(?=[\s\S]*\nStatus:)(?=[\s\S]*\nType:)(?=[\s\S]*\nContent-Type:)(?=[\s\S]*\nCreated:))'
args: ['--negate', '--multiline']
Expand All @@ -41,19 +85,19 @@ repos:
files: '^pep-\d+\.(rst|txt)$'
types: [text]
- id: validate-status
name: "Validate PEP Status field"
name: "Validate PEP 'Status' field"
language: pygrep
entry: '^Status:(?:(?! +(Draft|Withdrawn|Rejected|Accepted|Final|Active|Provisional|Deferred|Superseded|April Fool!)$))'
files: '^pep-\d+\.(rst|txt)$'
types: [text]
- id: validate-type
name: "Validate PEP Type field"
name: "Validate PEP 'Type' field"
language: pygrep
entry: '^Type:(?:(?! +(Standards Track|Informational|Process)$))'
files: '^pep-\d+\.(rst|txt)$'
types: [text]
- id: validate-content-type
name: "Validate PEP Content-Type field"
name: "Validate PEP 'Content-Type' field"
language: pygrep
entry: '^Content-Type:(?:(?! +text\/x-rst$))'
files: '^pep-\d+\.(rst|txt)$'
Expand All @@ -65,19 +109,19 @@ repos:
files: '^pep-\d+\.(rst|txt)$'
types: [text]
- id: validate-created
name: "Validate created dates"
name: "Validate PEP 'Created' field"
language: pygrep
entry: '^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])( \([^()]+\))?$))'
files: '^pep-\d+\.(rst|txt)$'
types: [text]
- id: validate-python-version
name: "Validate PEP Python-Version field"
name: "Validate PEP 'Python-Version' field"
language: pygrep
entry: '^Python-Version:(?:(?! +( ?[1-9]\.([0-9][0-9]?|x)(\.[1-9][0-9]?)?\??,?)+( \([^()]+\))?$))'
files: '^pep-\d+\.(rst|txt)$'
types: [text]
- id: validate-resolution
name: "Validate PEP Resolution field"
name: "Validate PEP 'Resolution' field"
language: pygrep
entry: '(?<!\n\n)^Resolution: (?:(?!(https:\/\/\S*|:pep:`[a-zA-Z\d \-<>#]*?`)\n))'
args: ['--multiline']
Expand Down
Empty file modified pep-0418/clockutils.py
100644 → 100755
View file
Open in desktop
Empty file.
2 changes: 1 addition & 1 deletion scan-ops.py → pep-0465/scan-ops.py
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
#!/usr/bin/env python3
# http://legacy.python.org/dev/peps/pep-0465/
# https://www.python.org/dev/peps/pep-0465/
Copy link
Member

Choose a reason for hiding this comment

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

Was "legacy" legacy at the point this was written? If so I think we should use a wayback machine link.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope; I looked up the history on this and it was added during a period when the main site was hosted on the new system but some things, like the PEPs, were still on the old one and in the process of transitioning. This is simply a link to the PEP, so since I was moving the file anyway, it makes sense to use the most up to date link as of now.

# https://gist.github.com/njsmith/9157645

# usage:
Expand Down
2 changes: 1 addition & 1 deletion pep-0590.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@ References
.. [2] tp_call/PyObject_Call calling convention
https://docs.python.org/3/c-api/typeobj.html#c.PyTypeObject.tp_call
.. [3] Using PY_VECTORCALL_ARGUMENTS_OFFSET in callee
https://github.com/markshannon/cpython/blob/vectorcall-minimal/Objects/classobject.c#L53
https://github.com/markshannon/cpython/blob/815cc1a30d85cdf2e3d77d21224db7055a1f07cb/Objects/classobject.c#L53
CAM-Gerlach marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Is this guaranteed to exist forevermore? I'd prefer to use a wayback machine link over this at the time it was written.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, at least pretty forevermore. GitHub describes them as permanent links and permalinks:

https://docs.github.com/en/repositories/working-with-files/using-files/getting-permanent-links-to-files

A big downside of the Wayback Machine is it's much slower to load than GitHub.

And there doesn't appear to be any Wayback Machine link for that page, let alone for when the PEP was written.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the link, sounds reasonable. (I'd probably prefer that authors used tags or similar going forwards, but for "fixing" previous this may be the least worst solution).

A

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd probably prefer that authors used tags or similar going forwards, but for "fixing" previous this may be the least worst solution.

Tags are mutable and can change or be deleted, and require the user to either find a suitable tag (if one exists), or (if they control the repo) create one, which is far more effort than simply pressing the y key. That's why always using proper permalinks is the standard, best practice approach for links that are, well, permanent. So long as authors are using inline links correctly (which we will make sure they will, going forward) what is the downside to explicitly specifying a commit hash?

.. [4] Argument Clinic
https://docs.python.org/3/howto/clinic.html

Expand Down
4 changes: 2 additions & 2 deletions pep-0635.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,8 @@ but also on the value of some class attributes, like the ``BinOp``
example above. The Visitor pattern is insufficiently flexible for
this: it can only select based on the class.

For a complete example, see
https://github.com/gvanrossum/patma/blob/master/examples/expr.py#L231
See a `complete example
Copy link
Member

Choose a reason for hiding this comment

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

Could you only change the link here. See preference for wayback above.

Copy link
Member Author

Choose a reason for hiding this comment

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

I normally would have, but it makes the link much longer, "messier" and harder to read if as plain text, both in the source and in the rendered HTML, so I made it a standard inline link to perhaps better preserve the spirit, if not the literal text of the author's intent. I don't think Guido would object to that, but we can ask him if you really think its needed.

<https://github.com/gvanrossum/patma/blob/be5969442d0584005492134c3b24eea408709db2/examples/expr.py#L231>`_.

Like the Visitor pattern, pattern matching allows for a strict separation
of concerns: specific actions or data processing is independent of the
Expand Down
2 changes: 1 addition & 1 deletion pep-0642.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1356,7 +1356,7 @@ they don't define ``__match_args__`` (the optimised fast path for the builtin
and standard library types named in :pep:`634` is retained).

Adapting the class matching example
`linked from PEP 635 <https://github.com/gvanrossum/patma/blob/master/examples/expr.py#L231>`_
`linked from PEP 635 <https://github.com/gvanrossum/patma/blob/be5969442d0584005492134c3b24eea408709db2/examples/expr.py#L231>`_
shows that for purely positional class matching, the main impact comes from the
changes to value constraints and name binding, not from the class matching
changes::
Expand Down
2 changes: 1 addition & 1 deletion pep-0675.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -819,7 +819,7 @@ and render HTML as-is is to use functions like ``mark_safe`` in
`Django
<https://docs.djangoproject.com/en/dev/ref/utils/#django.utils.safestring.mark_safe>`_
or ``do_mark_safe`` in `Jinja2
<https://github.com/pallets/jinja/blob/main/src/jinja2/filters.py#L1264>`_,
<https://github.com/pallets/jinja/blob/077b7918a7642ff6742fe48a32e54d7875140894/src/jinja2/filters.py#L1264>`_,
which cause XSS vulnerabilities:

::
Expand Down