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

tools: add GitHub Action linter for pr-url #37221

Merged
merged 1 commit into from
Feb 9, 2021
Merged

tools: add GitHub Action linter for pr-url #37221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 9 additions & 0 deletions .github/workflows/linters.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,3 +98,12 @@ jobs:
- uses: mszostok/[email protected]
with:
checks: "files,duppatterns"
lint-pr-url:
if: ${{ github.event.pull_request }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if: ${{ github.event.pull_request }}
if: github.event.pull_request.draft == false

Isn't this supposed to be the check here (based on the above jobs)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I meant to check only pull request (it wouldn't work on push, because it needs a PR URL), and I believe it's fine to have it running on draft PRs because it doesn't produce errors.

runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
with:
fetch-depth: 2
# GH Actions squashes all PR commits, HEAD^ refers to the base branch.
- run: git diff HEAD^ HEAD -G"pr-url:" -- "*.md" | ./tools/lint-pr-url.mjs ${{ github.event.pull_request.html_url }}
37 changes: 37 additions & 0 deletions tools/lint-pr-url.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
#!/usr/bin/env node

// Usage:
// git diff upstream/master...HEAD -G"pr-url:" -- "*.md" | \
// ./tools/lint-pr-url.mjs <expected-pr-url>

import process from 'node:process';
import readline from 'node:readline';
Comment on lines +7 to +8
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import process from 'node:process';
import readline from 'node:readline';
import readline from 'readline';

nit: since process is a part of the global object
question: why are we prepending 'node:' to the imported module names?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nit: since process is a part of the global object

I try to not rely on Node.js-only globals when writing ES modules to ease a potential port to the browser. I really don't feel strongly about that, and am willing to remove the import if anyone disagrees.

question: why are we prepending 'node:' to the imported module names?

Every time we try to introduce a new module in core comes the question of how breaking it would be for the ecosystem (is there a module that use the same name out there). It's been discussed several time to add a prefix to core modules, and with ESM we had to come up with a URL scheme to be spec compliant. TSC has decided that it will be ported to the CJS resolver as well (see #36098), so why not use it.
Again, I don't feel strongly about that either, if the consensus is not to use the prefix, that'd be fine with me.


const [, , expectedPrUrl] = process.argv;

const fileDelimiter = /^\+\+\+ b\/(.+\.md)$/;
const changeDelimiter = /^@@ -\d+,\d+ \+(\d+),\d+ @@/;
const prUrlDefinition = /^\+\s+pr-url: (.+)$/;

const validatePrUrl = (url) => url == null || url === expectedPrUrl;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const validatePrUrl = (url) => url == null || url === expectedPrUrl;
const validatePrUrl = (url) => url === expectedPrUrl;


let currentFile;
let currentLine;

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let currentPrUrl;

const diff = readline.createInterface({ input: process.stdin });
for await (const line of diff) {
if (fileDelimiter.test(line)) {
currentFile = line.match(fileDelimiter)[1];
console.log(`Parsing changes in ${currentFile}.`);
} else if (changeDelimiter.test(line)) {
currentLine = Number(line.match(changeDelimiter)[1]);
} else if (!validatePrUrl(line.match(prUrlDefinition)?.[1])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
} else if (!validatePrUrl(line.match(prUrlDefinition)?.[1])) {
} else if (currentPrUrl = line.match(prUrlDefinition)?.[1] &&
validatePrUrl(currentPrUrl)) {

console.warn(
`::warning file=${currentFile},line=${currentLine++},col=${line.length}` +
'::pr-url doesn\'t match the actual PR URL.'
);
} else if (line[0] !== '-') {
// Increment line counter if line is not being deleted.
currentLine++;
}
}