-
-
Notifications
You must be signed in to change notification settings - Fork 80
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Document contributor process for adding witness hint templates #933
Conversation
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 good! Just some wording nitpicks here and there.
CONTRIBUTING.md
Outdated
#### Adding a witness | ||
|
||
**Witnesses** are a new, unstable feature of `cargo-semver-checks` that let us create witness | ||
code, a minimal compilable example of potential downstream breakage\. They are configured via the |
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'm curious, what's the reason for the \.
everywhere? It's not something I've seen before.
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.
Just a bad regex find-and-replace for replacing .
with .
(extra spaces between periods). 😅
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.
Ah okay, no worries. You might need to edit my suggestions post-merge then, since I mirrored the \.
style.
CONTRIBUTING.md
Outdated
{{name}}(...); | ||
``` | ||
|
||
which could render to something like: |
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.
"Render" is a bit ambiguous here.
which could render to something like: | |
which in the RON format could be represented like: |
CONTRIBUTING.md
Outdated
the correct witness hints\. Note that it may contain output for other test crates - this | ||
not necessarily an error: see the troubleshooting section for more info\. Once you've | ||
verified that the `--- actual ---` results are as expected, copy the actual results | ||
into the `test_outputs/witnesses/<lint_name>.output.ron` file and save\. Running |
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.
Is there a viable way to insta
this? 🤔
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.
Totally! I was matching the behavior of the basic lint tests, but we could definitely use insta
here, and it would probably be better for contributor experience (but less consistent)
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 imagine we'll eventually move the existing non-insta steps to insta, so probably best to start standardizing toward the new behavior we'll want eventually anyway. If someone is annoyed at the inconsistency, we can tell them we'd love a PR to move everything to insta 😅
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.
OK, I'll open a PR to use insta for the tests instead, then update the docs 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.
fed498d
to
1651ed1
Compare
Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
1b4de34
to
c20c5db
Compare
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.
Awesome 🚀
Adds information about the
witness
field and when and how to add a hint template. Also sets thewitness
toNone
inmake_new_lint.sh
, with a link to this new section of the contributor docs.Rendered view