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

tools: strip comments from lint-md rollup output #41092

Merged
merged 1 commit into from
Dec 7, 2021
Merged

tools: strip comments from lint-md rollup output #41092

merged 1 commit into from
Dec 7, 2021

Conversation

Trott
Copy link
Member

@Trott Trott commented Dec 5, 2021

@Trott Trott requested a review from targos December 5, 2021 16:29
@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Dec 5, 2021
@Trott Trott mentioned this pull request Dec 5, 2021
Closed
@Trott Trott added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Dec 7, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 7, 2021
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/41394/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/41401/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/41405/

BridgeAR
BridgeAR reviewed Dec 7, 2021
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

Are we certain we keep all necessary copyright / license blocks intact?

@Trott
Copy link
Member Author

Trott commented Dec 7, 2021

Are we certain we keep all necessary copyright / license blocks intact?

Unfortunately, that's a good question. And, in fact, I'm not sure we aren't in license violation already by using rollup but not distributing LICENSE files that come with the code. If that's a problem, then we may need to consider either checking in the entire node_modules directory (like we do for ESLint) or else going back to not shipping the markdown linter with the repo at all and requiring installation to use.

@Trott
Copy link
Member Author

Trott commented Dec 7, 2021

Are we certain we keep all necessary copyright / license blocks intact?

Unfortunately, that's a good question. And, in fact, I'm not sure we aren't in license violation already by using rollup but not distributing LICENSE files that come with the code. If that's a problem, then we may need to consider either checking in the entire node_modules directory (like we do for ESLint) or else going back to not shipping the markdown linter with the repo at all and requiring installation to use.

I did a grep for "Permission" and "Copyright" and came up empty. It also leaves @license JSDoc entries intact. So it seems that we're good. It does not appear to be removing license text or copyright text.

@Trott
Copy link
Member Author

Trott commented Dec 7, 2021

So it seems that we're good. It does not appear to be removing license text or copyright text.

Although that doesn't answer the question as to whether we should be including LICENSE files etc. But if so, that's already a problem. This PR does not exacerbate it, at least.

@Trott Trott added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 7, 2021
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 7, 2021
@nodejs-github-bot nodejs-github-bot merged commit c93a639 into nodejs:master Dec 7, 2021
@nodejs-github-bot
Copy link
Collaborator

Landed in c93a639

@Trott Trott deleted the strip-comments branch December 7, 2021 23:51
This was referenced Dec 8, 2021
Open
Open
danielleadams pushed a commit that referenced this pull request Dec 13, 2021
@Trott @danielleadams
tools: strip comments from lint-md rollup output
6052b39 
Refs: #41081 (comment)

PR-URL: #41092
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
@github-actions github-actions bot mentioned this pull request Dec 14, 2021
Open
46 tasks
danielleadams pushed a commit that referenced this pull request Dec 14, 2021
@Trott @danielleadams
tools: strip comments from lint-md rollup output
40a773a 
Refs: #41081 (comment)

PR-URL: #41092
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
@danielleadams danielleadams mentioned this pull request Dec 14, 2021
Merged
@github-actions github-actions bot mentioned this pull request Dec 15, 2021
Open
24 tasks
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
@Trott @danielleadams
tools: strip comments from lint-md rollup output
3e41c35 
Refs: #41081 (comment)

PR-URL: #41092
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
@Trott @danielleadams
tools: strip comments from lint-md rollup output
f045d8e 
Refs: #41081 (comment)

PR-URL: #41092
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
@Trott @Linkgoron
tools: strip comments from lint-md rollup output
fa3b1f7 
Refs: nodejs#41081 (comment)

PR-URL: nodejs#41092
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
@Trott @danielleadams
tools: strip comments from lint-md rollup output
2c28f86 
Refs: #41081 (comment)

PR-URL: #41092
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR left review comments

@tniessen tniessen tniessen approved these changes

@targos targos targos approved these changes

@lpinca lpinca lpinca approved these changes

@bnb bnb bnb approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.
7 participants
@Trott @nodejs-github-bot @bnb @lpinca @targos @tniessen @BridgeAR