GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — tools: add GitHub Action linter for pr-url by aduh95 · Pull Request #37221 · nodejs/node

Conversation

@aduh95

@aduh95 aduh95 commented Feb 4, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

The PR URL is unknown when someone starts working on a change, they may
be tempted to use a placeholder with a "fake" PR URL value to make the
linter pass on their local machine. This commit adds warnings when a
pr-url doesn't correspond to the actual PR URL to help reviewers spot
those "fake" URLs before landing.

screen capture

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ExE-Boss and RaisinTen reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@nodejs-github-bot nodejs-github-bot added meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. labels Feb 4, 2021
@aduh95 aduh95 added the wip Issues and PRs that are still a work in progress. label Feb 4, 2021
@aduh95
aduh95 marked this pull request as draft February 4, 2021 13:08
@aduh95
aduh95 force-pushed the pr-url-lint branch from 918b923 to db0d74c Compare February 4, 2021 14:38
@aduh95
aduh95 marked this pull request as ready for review February 4, 2021 14:40
@aduh95 aduh95 closed this Feb 4, 2021
@aduh95 aduh95 reopened this Feb 4, 2021
@aduh95 aduh95 removed the wip Issues and PRs that are still a work in progress. label Feb 4, 2021
richardlau
richardlau reviewed Feb 4, 2021
View reviewed changes

@richardlau richardlau left a comment

Copy link
Copy Markdown
Member

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.

Will this result in false positives for the release PR's (e.g. see the doc changes in #36476)?

Also I wonder if it's possible to detect/pass the expected pr-url into the existing YAML linting done in https://github.com/nodejs/remark-preset-lint-node/blob/0b2a13018de6ad9bcd234e4420bba03ae9f42c29/remark-lint-nodejs-yaml-comments.js#L134-L139 (see https://github.com/nodejs/node/runs/1825250022#step:5:11 for an example of a detected malformed pr-url)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@aduh95

aduh95 commented Feb 4, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

Will this result in false positives for the release PR's (e.g. see the doc changes in #36476)?

Yes, it would print out warnings, but the workflow would still end with a green ticks – it only outputs warnings and not errors (so it wouldn't interfere with ncu or the commit queue).
IMHO it's worth it, it's easy to miss this with human eyes and we end up with PR like #37205 from time to time.

Also I wonder if it's possible to detect/pass the expected pr-url into the existing YAML linting

I remember trying to do that when I added the YAML linting a while back, but couldn't find a way: the linter parses all .md files of the code base, and doesn't know which lines are being add in a specific PR (nor the PR URL, but that could be set up in an environment variable).
richardlau reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
richardlau
richardlau approved these changes Feb 4, 2021
View reviewed changes
RaisinTen
RaisinTen reviewed Feb 4, 2021
View reviewed changes
Comment thread tools/lint-pr-url.mjs Outdated
Comment on lines 7 to 8

@RaisinTen RaisinTen Feb 4, 2021

Copy link
Copy Markdown
Member

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.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@aduh95 aduh95 Feb 4, 2021

Copy link
Copy Markdown
Contributor Author

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ExE-Boss and RaisinTen reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
RaisinTen
RaisinTen requested changes Feb 5, 2021
View reviewed changes

@RaisinTen RaisinTen left a comment

Copy link
Copy Markdown
Member

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 currentLine gets incremented over parts of the diff log where
line is deleted (prepended with -s) whereas we want currentLine
to remain the same while traversing deletions. Hence, the warning will
point to incorrect line numbers though line holds the correct value.
How should we fix this?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aduh95 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
RaisinTen
RaisinTen reviewed Feb 6, 2021
View reviewed changes
Comment thread tools/lint-pr-url.mjs Resolved

Uh oh!

There was an error while loading. Please reload this page.
RaisinTen
RaisinTen reviewed Feb 6, 2021
View reviewed changes
Comment thread tools/lint-pr-url.mjs Resolved

Uh oh!

There was an error while loading. Please reload this page.
@aduh95
aduh95 requested a review from RaisinTen February 8, 2021 10:34
RaisinTen
RaisinTen approved these changes Feb 8, 2021
View reviewed changes
RaisinTen
RaisinTen reviewed Feb 8, 2021
View reviewed changes
Comment thread tools/lint-pr-url.mjs Outdated

@RaisinTen RaisinTen Feb 8, 2021

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
RaisinTen
RaisinTen reviewed Feb 8, 2021
View reviewed changes
Comment thread tools/lint-pr-url.mjs Outdated

@RaisinTen RaisinTen Feb 8, 2021

Copy link
Copy Markdown
Member

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.

Suggested change
let currentPrUrl;

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
RaisinTen
RaisinTen reviewed Feb 8, 2021
View reviewed changes
Comment thread tools/lint-pr-url.mjs Outdated

@RaisinTen RaisinTen Feb 8, 2021

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@RaisinTen

RaisinTen commented Feb 8, 2021

Copy link
Copy Markdown
Member

@aduh95 currently, validatePrUrl returns true for null, so, no warning is thrown for lines like:

+ pr-url:

Added some suggestions to fix that.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
RaisinTen
RaisinTen reviewed Feb 8, 2021
View reviewed changes
Comment thread .github/workflows/linters.yml Outdated

@RaisinTen RaisinTen Feb 8, 2021

Copy link
Copy Markdown
Member

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.

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)?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@aduh95 aduh95 Feb 8, 2021

Copy link
Copy Markdown
Contributor Author

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
RaisinTen reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@aduh95

aduh95 commented Feb 8, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@aduh95 currently, validatePrUrl returns true for null, so, no warning is thrown for lines like:

+ pr-url:

Added some suggestions to fix that.

That doesn't seem necessary, that kind of checks are already made by the linter, it would produce a warning on top of the error from lint-md.
RaisinTen reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RaisinTen

RaisinTen commented Feb 8, 2021

Copy link
Copy Markdown
Member

Can we disable lint-md from doing such checks?
I was considering adding this change here because lint-md only runs on PRs
that are ready for review, so pr-urls wouldn't be linted completely by this tool:
https://github.com/nodejs/node/blob/d75f3e97ff92025c184cc84606a8438f01862bcb/.github/workflows/linters.yml#L42-L43
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95

aduh95 commented Feb 8, 2021

Copy link
Copy Markdown
Contributor Author

Can we disable lint-md from doing such checks?
I was considering adding this change here because lint-md only runs on PRs
that are ready for review, so pr-urls wouldn't be linted completely by this tool:

If you open a PR to re-enable linter GH actions for draft PR, I'd gladly approve it (I'm with with on that, I think it makes opening draft PR useless). I don't think it's a good idea to make lint-md more permissive, I've added this rule because there were quite a few mistakes that was spotted only when the rule was added; removing it would almost certainly mean we'll start seeing a few of those mistakes hoping here and there.
RaisinTen reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Feb 8, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/36016/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RaisinTen RaisinTen added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Feb 8, 2021
@nodejs-github-bot

nodejs-github-bot commented Feb 8, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/36024/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Trott
Trott approved these changes Feb 9, 2021
View reviewed changes
@nodejs-github-bot

nodejs-github-bot commented Feb 9, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/36028/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95
tools: add GitHub Action linter for pr-url
33d3a2e 
PR-URL: nodejs#37221
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@aduh95
aduh95 force-pushed the pr-url-lint branch from d75f3e9 to 33d3a2e Compare February 9, 2021 10:45
@aduh95

aduh95 commented Feb 9, 2021

Copy link
Copy Markdown
Contributor Author

Landed in 33d3a2e
RaisinTen reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95
aduh95 merged commit 33d3a2e into nodejs:master Feb 9, 2021
@aduh95
aduh95 deleted the pr-url-lint branch February 9, 2021 10:46
danielleadams pushed a commit that referenced this pull request Feb 16, 2021
@aduh95 @danielleadams
tools: add GitHub Action linter for pr-url
6db5e79 
PR-URL: #37221
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
This was referenced Feb 16, 2021
v15.9.0 proposal #37405
Closed
v15.9.0 proposal #37406
Merged
targos pushed a commit that referenced this pull request May 1, 2021
@aduh95 @targos
tools: add GitHub Action linter for pr-url
981659c 
PR-URL: #37221
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@danielleadams danielleadams mentioned this pull request May 3, 2021
v14.17.0 proposal #38507
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RaisinTen RaisinTen RaisinTen approved these changes
@Trott Trott Trott approved these changes
@richardlau richardlau richardlau approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@aduh95 @RaisinTen @nodejs-github-bot @Trott @richardlau
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.