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.
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 minor branding guidelines to
CONTRIBUTING.md
#21495[docs] add minor branding guidelines to
CONTRIBUTING.md
#21495Changes from 13 commits
f96e8b1
21b2344
5c3ad2a
819cb2f
90a4131
db38684
1627cd6
b488bc4
95b0ede
e618c91
3347960
93405fb
18ee73d
23117ca
54639cd
4448af3
23b1655
2079206
3c4b46a
057829d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Yep exactly what I had in mind. Because this not only ensures that what we expect to be reported as errors are indeed reported as errors, but also checks that what we don't expect to be reported as errors and expect to be valid exceptions… isn't accidentally reported as errors either. So this should cover all 4 cases I mentioned above.
Btw, now that we have this implemented as a
spec
, I think it's worth removing the ❌ and ✅ at the end of each lines of your fixtures now, as they are not a needed hack anymore, and could accidentally mislead the thing being tested (e.g. imagine if our implementation offind_tool_name_formatting_errors
didn't check if the lines ofCONTRIBUTING.md
started with "- ❌", but instead our implementation tested ifinclude?("❌")
… we don't want those emojis at end of line to mud the waters 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.
I thought about removing them but ended up leaving them (hence why I reworded the header of the fixture file), because I think it's still useful when we need to update this fixture in the future. I think the header explains all that so there's no "accidentally misleading" I think 🤔 unless devs don't read it. But you have a good point nonetheless, I'll remove them