-
Notifications
You must be signed in to change notification settings - Fork 547
issue_130_7 implementing new features #280
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
Conversation
Thanks @rajcspsg! I'll take a look soon. |
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 @rajcspsg 👍
I think we should remove the parts that are aimed more at team members than general contributors. I've tried to leave a comment at the parts I think we can remove/change.
Also, you will need to hard-wrap all the lines to respect the 100 char limit.
src/implementing_new_feature.md
Outdated
|
||
You don't have to have the implementation fully ready for r+ to ask for a pFCP, but it is generally a good idea to have at least a proof of concept so that people can see what you are talking about. | ||
|
||
That starts a "proposed final comment period" (pFCP), which requires all members of the team to sign off the FCP. After they all do so, there's a week long "final comment period" where everybody can comment, and if no new concerns are raised, the PR/issue gets FCP approval. |
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 believe it is now "all but two"? cc @Mark-Simulacrum
Co-Authored-By: rajcspsg <[email protected]>
Co-Authored-By: rajcspsg <[email protected]>
Co-Authored-By: rajcspsg <[email protected]>
Co-Authored-By: rajcspsg <[email protected]>
Co-Authored-By: rajcspsg <[email protected]>
Did you want to rename the file to be plural as well? |
Also, please don't forget all the comments that Github "helpfully" collapsed (there are 8). |
b31c100
to
5cf4b1a
Compare
5cf4b1a
to
032e457
Compare
@mark-i-m @shepmaster I updated the book for the changes requested. Please review and let me know your comments. |
@shepmaster Do you know how many people are required to check off on pFCP to get to FCP? |
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.
LGTM :)
I left a few minor nits, but other than that r=me.
I would still like to hear back from somebody who knows what it takes to start FCP. (@shepmaster?)
Co-Authored-By: rajcspsg <[email protected]>
Co-Authored-By: rajcspsg <[email protected]>
Co-Authored-By: rajcspsg <[email protected]>
Thanks! |
@rajcspsg Thanks for reminding me. Going through the list in #98:
I think this is reasonably covered in what you already wrote.
Generally, these are normal
For example, consider this test that checks that The point is just to make sure that the feature is not usable without the feature gate. When the feature is stabilized, the test can be deleted.
Hmmm... I'm not sure what Niko meant by this.
This seems to already be covered in what you wrote. |
Thanks @mark-i-m We are good with everything expect |
@rajcspsg I'm not really sure what Niko meant by that. I think what we have is good enough for now, but I left a comment on the other thread asking for Niko's input. If you want to open a PR about feature gate tests, that would be appreciated. |
@mark-i-m I have created new section implementing new features based on the forge document.
Also, I'm not sure about the how to add a test covering your feature gate, change for simple syntactic things and doing more advanced gates by looking at
sess.features.your_feature_here
as a boolean for the issue #98.