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 11 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.
The issue with this is that you're only checking if lines not ending with ❌ are not detected at errors (actually, talk about double-negation… had it wrong the first time I read that code and thought it was doing the opposite at first…), but you're not checking if all the lines ending with ✅ are detected as errors.
If you rewrite this as a spec (as suggested in my top-level comment from a couple of minutes ago), you should
expect(errors).to equal(the_exact_expected_list)
, as opposed to do it similar to what you did it, only expecting that allerrors
end with ✅ but without checking that you didn't miss one of them in the original fixture.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.
Your suggestion for helpers (similar to plugins) makes sense! It's literally the only file we have in the helper folder, nice 🤓 so this can definitely be moved to rspecs and proper unit tests be written, but can you also help me understand what you meant in this message you sent above? I think the tests are covering all lines, whether they're successful or not 🤔 We do check if the ones with ✅ succeed (and they do), and we also check if the ones with ❌ fail (and they do). What's missing? (just trying to understand here, if we were to keep it, although we won't 😇 )
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.
Well another way of seeing this is that there are 4 categories of lines:
find_tool_name_formatting_errors
as suchfind_tool_name_formatting_errors
as errorsfind_tool_name_formatting_errors
find_tool_name_formatting_errors
What we want is to ensure not only that cases 1 and 2 happen (i.e. our implementation work when it's supposed to) but also that cases 3 and 4 don't happen (i.e. that our implementation doesn't create neither false positives or false negatives)
In your code above, you only look at if the return value of
find_tool_name_formatting_errors
(akaerrors
), so you're only considering cases 1 and 4 (i.e. things that have been reported aserrors
). And especially then you're raising aUI.error
if case 4 (the thing that was raised as errors shouldn't have been) happens.But you're not checking case 3 at all, i.e. lines that should be considered as errors and should have been returned in
errors
when you calledfind_tool_name_formatting_errors
, but weren't reported (in case our implementation offind_tool_name_formatting_errors
would be incorrect — which is what we want to test here after all)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.
Good point! 🎯