-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
docs: add snapshot-related info sections to the Pull Requests documentation #9302
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: add snapshot-related info sections to the Pull Requests documentation #9302
Conversation
Thanks for the PR, @alythobani! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 45700e0. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
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.
A great start, thanks! I like the fixing up of header levels and the general explanations. Clear and informative. Nicely done! 🙌
docs/contributing/Pull_Requests.mdx
Outdated
|
||
### Updating Snapshots | ||
|
||
Jest snapshots are generated for use in some tests, e.g. for [rule schemas](https://github.com/typescript-eslint/typescript-eslint/tree/main/packages/eslint-plugin/tests/schema-snapshots) and [code examples in rule docs](https://github.com/typescript-eslint/typescript-eslint/tree/main/packages/eslint-plugin/tests/docs-eslint-output-snapshots). You may need to re-generate these snapshots after adjusting a rule and/or its documentation, by running the relevant test suite(s) with the `-u` flag: |
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.
.../main/...
[Docs] It's best to use commit permalinks, as file intent and/or paths can change over time. E.g. https://github.com/typescript-eslint/typescript-eslint/tree/04990d545fc119329551ae3a55d79dfc0c7bf147/packages/eslint-plugin/tests/schema-snapshots
.
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.
@JoshuaKGoldberg I slightly prefer linking to main
in this case:
- If the file is missing, we want to update the docs
- If we updated the code, we don't want anyone to read outdated code.
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.
Hmm, I guess that makes sense. I feel more comfortable about these kinds of permalinks when Docusaurus flags on missing links...
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.
Happy to do whatever we think is best here!
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.
✨ looks great to me, thanks!
3bb4f1d
into
typescript-eslint:main
I say, let's just ship 😄. If the repo ever decides on a standardized approach we'd lint for it. Thanks again @alythobani! |
PR Checklist
Overview
🌱