-
Notifications
You must be signed in to change notification settings - Fork 16
Defects vs subsets #127
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
base: main
Are you sure you want to change the base?
Defects vs subsets #127
Conversation
…n explicit Amplification section exists (instead, whatever paragraph immediately opens the guideline is just normative). We can continue to call it that but the subheading is not necessary.
Co-authored-by: Pete LeVasseur <[email protected]>
✅ Deploy Preview for scrc-coding-guidelines ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
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.
Thanks @AlexCeleste for the excellent additions! Could you take a look at the comments I left?
By the way -- I think I recall you saying the plan is to incorporate these examples more into the Style Guideline to help flesh it out. Was that right? In this PR or another?
I suppose then it would be quite helpful to use the examples to illustrate the difference between a defect and subset.
@@ -308,15 +304,27 @@ what it covers. | |||
Content **SHOULD** aim to be as short and self-contained as possible, while still explaining | |||
the scope of the guideline. | |||
|
|||
Content **SHOULD NOT** cover the rationale for the guideline, which is done in the ``rationale`` section. |
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 think you mentioned about removing the IETF RFC 2119-styling to this chapter when it was intended for readers. Now that it's intended for contributors are you thinking to keep it in?
Or is this change unrelated to that to aid in understanding?
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.
Perhaps this was rewritten in order to read better? Curious to hear your thoughts.
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.
Did you have some thoughts on this? Could we make use of MUST and MAY to specify which contents must be included and which ones are optional?
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've checked the additions a few times over the past weeks and they look quite good to me overall.
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.
Left a couple of thoughts! Thanks a bunch for sweeping through and addressing comments @AlexCeleste
@@ -308,15 +304,27 @@ what it covers. | |||
Content **SHOULD** aim to be as short and self-contained as possible, while still explaining | |||
the scope of the guideline. | |||
|
|||
Content **SHOULD NOT** cover the rationale for the guideline, which is done in the ``rationale`` section. |
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.
Perhaps this was rewritten in order to read better? Curious to hear your thoughts.
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.
Missed a comment. Realized we can also use the linking to the standard library for core
and other standard library crates as well 🥳
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 think we're almost there! Last question about choice of ETF RFC 2119-styling on one section.
@@ -308,15 +304,27 @@ what it covers. | |||
Content **SHOULD** aim to be as short and self-contained as possible, while still explaining | |||
the scope of the guideline. | |||
|
|||
Content **SHOULD NOT** cover the rationale for the guideline, which is done in the ``rationale`` section. |
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.
Did you have some thoughts on this? Could we make use of MUST and MAY to specify which contents must be included and which ones are optional?
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 adds a basic example of three kinds of guideline: