-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Security cookbook pass #1825
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
Merged
Merged
Security cookbook pass #1825
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
5c27ee7
remove unwanted comma
greg0ire 7e4a506
This abbreviation felt sloppy; write like a sir
greg0ire 6f22829
Add some punctuation here and there.
greg0ire 128333e
Try to produce something easier to understand.
greg0ire c33c26a
avoid awkward awkward construction
greg0ire bd26b41
Add some punctuation
greg0ire d4f661f
It does not feel right without -ing
greg0ire faaac70
Improve formatting
greg0ire File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
This comma is not necessary.
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 not sure what was meant here: who is using the
requires_channel
? The developer who writes the code (which made me add this comma), or is it the access_control rules? But maybe it does not even matter. What do you think?If you really want me to remove this comma, how should I do this? Should I add a reverting commit to this pull request? Or make another pull request?
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 not really sure, but I think that doing an stop there is odd. That's why I suggest that.
If you revert you'll lose the other change, so simple do a different PR.
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.
It feels less odd to me without the comma, so if you're not sure, I propose we let @weaverryan decide, I believe he's a native english speaker.
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.
Fine! :)
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.
It is actually better without the comma - it's just one straight thought, similar to
I got to the hotel using a taxi
.But, the vast majority of what you had here is perfect.
Cheers!
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.
@Nomack84 : looks like you were right :) !
@weaverryan : I trust I don't have anything do to, it looks like you managed apply the PR without this commit...