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.

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

docs: copy & use main package version in docs on release #16252

Merged
merged 4 commits into from Sep 5, 2022

Conversation

jugalthakkar
Copy link
Contributor

@jugalthakkar jugalthakkar commented Aug 27, 2022

Prerequisites checklist

What is the purpose of this pull request?

[x] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Fixes #16212
Remove dependency on root package.json by generating eslintVersion.js with the version hardcoded

Is there anything you'd like reviewers to focus on?

Do we want to keep the ability to read the version from process.env? Else, we can altogether remove the eslintVersion.js file and always have it generated

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 27, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • login: jugalthakkar / name: Jugal Thakkar (213663a)

@eslint-github-bot eslint-github-bot bot added triage An ESLint team member will look at this issue soon documentation Relates to ESLint's documentation labels Aug 27, 2022
@netlify
Copy link

netlify bot commented Aug 27, 2022

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit ae326a1
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/6312abd7f07e4a000947a06d
😎 Deploy Preview https://deploy-preview-16252--docs-eslint.netlify.app/rules/id-denylist
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Makefile.js Outdated
@@ -282,6 +282,9 @@ function generateRelease() {
generateBlogPost(releaseInfo);
commitSiteToGit(`v${releaseInfo.version}`);

echo("Generating eslintVersion.js for docs");
fs.writeFileSync("docs/src/_data/eslintVersion.js", `module.exports = "${releaseInfo.version}";`);
Copy link
Member

@kecrily kecrily Aug 27, 2022

Choose a reason for hiding this comment

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

In the current version, the eslint version can be passed in via environment variables. Maybe we should keep this feature.

ref

const { ESLINT_VERSION } = process.env;

Copy link
Contributor Author

@jugalthakkar jugalthakkar Aug 27, 2022

Choose a reason for hiding this comment

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

if line 286 was instead

process.env.ESLINT_VERSION = releaseInfo.version

would that also lead to the version in the docs being the expected one? Sorry for the dumb question, I am not very clear on the generate-release script and how the docs are actually generated neither was I able to run it locally

Copy link
Member

@kecrily kecrily Aug 28, 2022

Choose a reason for hiding this comment

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

No. This is because environment variables are passed in when running the documentation, not when executing generate-release.


how the docs are actually generated neither was I able to run it locally

What problems did you encounter? Just go directly to the docs directory and install the dependencies and run the scripts like a normal project.

Copy link
Member

@nzakas nzakas Aug 29, 2022

Choose a reason for hiding this comment

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

I don’t want to read from the environment or anywhere else. The version should be static to the repo so we can easily copy files elsewhere and have it work correctly.

Copy link
Member

@nzakas nzakas Aug 29, 2022

Choose a reason for hiding this comment

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

Oops sorry, @kecrily is right. We still do need to read from env in order to support the HEAD branch.

Copy link
Contributor Author

@jugalthakkar jugalthakkar Aug 30, 2022

Choose a reason for hiding this comment

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

ok, so I will leave the eslintVersion.js file as it is and also generate a static one during the generate-release script as in the PR. Is there any other change you would suggest in this PR?

Copy link
Member

@nzakas nzakas Aug 30, 2022

Choose a reason for hiding this comment

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

After thinking about this some more, here's what I think will probably work best:

  1. Update eslintVersion.js so that it reads from the package.json file in the docs/ directory instead of the one in the root directory. Otherwise, it will remain the same.
  2. In Makefile.js update the version field of docs/package.json to the newly-created release version.

Make sense?

Copy link
Contributor Author

@jugalthakkar jugalthakkar Aug 31, 2022

Choose a reason for hiding this comment

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

sounds good

Copy link
Contributor Author

@jugalthakkar jugalthakkar Aug 31, 2022

Choose a reason for hiding this comment

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

updated the pull request, if you could please review and provide your comments

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 31, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@jugalthakkar jugalthakkar changed the title docs: Hardcode eslintVersion.js on release docs: copy main package version to docs package on release and use that in docs Aug 31, 2022
@eslint-github-bot
Copy link

eslint-github-bot bot commented Aug 31, 2022

Hi @jugalthakkar!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The length of the commit message must be less than or equal to 72

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

@jugalthakkar jugalthakkar changed the title docs: copy main package version to docs package on release and use that in docs docs: copy & use main package version in docs on release Aug 31, 2022
@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Sep 2, 2022
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Can you also update the current version to "8.23.0"?

"version": "1.0.0",

Otherwise, when we merge this PR, v1.0.0 would appear in the version list on https://eslint.org/docs/head/

Makefile.js Outdated Show resolved Hide resolved
jugalthakkar and others added 2 commits Sep 3, 2022
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
@jugalthakkar
Copy link
Contributor Author

jugalthakkar commented Sep 3, 2022

Can you also update the current version to "8.23.0"?

"version": "1.0.0",

Otherwise, when we merge this PR, v1.0.0 would appear in the version list on https://eslint.org/docs/head/

Done

@jugalthakkar jugalthakkar requested a review from mdjermanovic Sep 3, 2022
Copy link
Member

@mdjermanovic mdjermanovic left a comment

LGTM, thanks!

nzakas
nzakas approved these changes Sep 5, 2022
@nzakas nzakas merged commit 1ae8236 into eslint:main Sep 5, 2022
19 of 20 checks passed
nzakas pushed a commit that referenced this pull request Sep 5, 2022
* docs: update docs package ver from main on release

fixes #16212

* docs: read docs package ver instead of main

fixes #16212

* docs: add newline to updated docs package json

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

* docs: update docs package json version

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
@jugalthakkar jugalthakkar deleted the issue16212 branch Sep 8, 2022
@jugalthakkar
Copy link
Contributor Author

jugalthakkar commented Sep 8, 2022

thanks, @nzakas @mdjermanovic 🙏
that marks my first open source contribution 🎉

btmills pushed a commit that referenced this pull request Sep 12, 2022
* fix: Ensure globbing doesn't include subdirectories

Fixes #16260

* docs: copy & use main package version in docs on release (#16252)

* docs: update docs package ver from main on release

fixes #16212

* docs: read docs package ver instead of main

fixes #16212

* docs: add newline to updated docs package json

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

* docs: update docs package json version

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

* chore: enable linting `.eleventy.js` again (#16274)

* Fix filtering of globs

* Enable partial matching of globs

Co-authored-by: Jugal Thakkar <2640821+jugalthakkar@users.noreply.github.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Sep 22, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | patch | [`8.23.0` -> `8.23.1`](https://renovatebot.com/diffs/npm/eslint/8.23.0/8.23.1) |

---

### Release Notes

<details>
<summary>eslint/eslint</summary>

### [`v8.23.1`](https://github.com/eslint/eslint/releases/tag/v8.23.1)

[Compare Source](eslint/eslint@v8.23.0...v8.23.1)

#### Bug Fixes

-   [`b719893`](eslint/eslint@b719893) fix: Upgrade eslintrc to stop redefining plugins ([#&#8203;16297](eslint/eslint#16297)) (Brandon Mills)
-   [`734b54e`](eslint/eslint@734b54e) fix: improve autofix for the `prefer-const` rule ([#&#8203;16292](eslint/eslint#16292)) (Nitin Kumar)
-   [`6a923ff`](eslint/eslint@6a923ff) fix: Ensure that glob patterns are normalized ([#&#8203;16287](eslint/eslint#16287)) (Nicholas C. Zakas)
-   [`c6900f8`](eslint/eslint@c6900f8) fix: Ensure globbing doesn't include subdirectories ([#&#8203;16272](eslint/eslint#16272)) (Nicholas C. Zakas)

#### Documentation

-   [`16cba3f`](eslint/eslint@16cba3f) docs: fix mobile double tap issue ([#&#8203;16293](eslint/eslint#16293)) (Sam Chen)
-   [`e098b5f`](eslint/eslint@e098b5f) docs: keyboard control to search results ([#&#8203;16222](eslint/eslint#16222)) (Shanmughapriyan S)
-   [`1b5b2a7`](eslint/eslint@1b5b2a7) docs: add Consolas font and prioritize resource loading ([#&#8203;16225](eslint/eslint#16225)) (Amaresh  S M)
-   [`1ae8236`](eslint/eslint@1ae8236) docs: copy & use main package version in docs on release ([#&#8203;16252](eslint/eslint#16252)) (Jugal Thakkar)
-   [`279f0af`](eslint/eslint@279f0af) docs: Improve id-denylist documentation ([#&#8203;16223](eslint/eslint#16223)) (Mert Ciflikli)

#### Chores

-   [`38e8171`](eslint/eslint@38e8171) perf: migrate rbTree to js-sdsl ([#&#8203;16267](eslint/eslint#16267)) (Zilong Yao)
-   [`1c388fb`](eslint/eslint@1c388fb) chore: switch nyc to c8 ([#&#8203;16263](eslint/eslint#16263)) (唯然)
-   [`67db10c`](eslint/eslint@67db10c) chore: enable linting `.eleventy.js` again ([#&#8203;16274](eslint/eslint#16274)) (Milos Djermanovic)
-   [`42bfbd7`](eslint/eslint@42bfbd7) chore: fix `npm run perf` crashes ([#&#8203;16258](eslint/eslint#16258)) (唯然)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

 **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xOTQuNSIsInVwZGF0ZWRJblZlciI6IjMyLjE5NC41In0=-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1543
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion documentation Relates to ESLint's documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docs: Hardcode eslintVersion.js on release
4 participants