Skip to content
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

update NEP template #474

Merged
merged 7 commits into from May 15, 2023
Merged

update NEP template #474

merged 7 commits into from May 15, 2023

Conversation

robert-zaremba
Copy link
Contributor

  • added Consequences and merged into it "considerations"
  • moved Alternatives and Future Possibilities above the Consequences / Considerations
  • Updated Changelog Concerns table
  • Added example of NEP Status section for a Pull Request description in the NEP process (nep-0001.md)

@robert-zaremba robert-zaremba requested a review from a team as a code owner April 3, 2023 21:49
@render
Copy link

render bot commented Apr 3, 2023

@robert-zaremba robert-zaremba added the documentation Improvements or additions to documentation label Apr 3, 2023
@@ -70,51 +70,67 @@ The section should return to the examples given in the previous section, and exp

[Explicitly outline any security concerns in relation to the NEP, and potential ways to resolve or mitigate them. At the very least, well-known relevant threats must be covered, e.g. person-in-the-middle, double-spend, XSS, CSRF, etc.]

## Drawbacks (Optional)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drawbacks is merged into consequences.

neps/nep-0001.md Outdated Show resolved Hide resolved
Moderator, when moving a NEP to review stage, should update the Pull Request description to include the
review summary, example:

```markdown
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a template to allow easy copy-paste

@@ -70,51 +70,67 @@ The section should return to the examples given in the previous section, and exp

[Explicitly outline any security concerns in relation to the NEP, and potential ways to resolve or mitigate them. At the very least, well-known relevant threats must be covered, e.g. person-in-the-middle, double-spend, XSS, CSRF, etc.]

## Drawbacks (Optional)
## Alternatives
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alternatives should be before final Consequences

Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this change does not flag any concerns from my side. There are a couple of tweaks here and there, so I lean towards approving it.

I would love to hear inputs from NEP authors and @ori-near before merging it.

neps/nep-0001.md Outdated Show resolved Hide resolved
| 1 | | | |
| 2 | | | |
> Template for Subject Matter Experts review for this version:
> Status: New | Ongoing | Resolved
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a need to separate "new" and "ongoing". Historically, we used "resolved" and "unresolved". Do you have concerns about those?

Note, most of the time, SMEs do not have permission to edit the NEP document (submitted PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, whatever makes sense. Let's check other opinions before updating.

Comment on lines +97 to +99
### Backwards Compatibility

[All NEPs that introduce backwards incompatibilities must include a section describing these incompatibilities and their severity. Author must explain a proposes to deal with these incompatibilities. Submissions without a sufficient backwards compatibility treatise may be rejected outright.]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, it is good to highlight this objective as a standalone section.

@frol frol added WG-unknown No Work Group is currently accountable S-draft/needs-moderator-review A NEP in the DRAFT stage that needs a moderator review. A-Housekeeping An update to the NEPs repo organization. and removed documentation Improvements or additions to documentation labels May 2, 2023
Copy link
Contributor

@ori-near ori-near left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for proposing these changes @robert-zaremba. The template flows better this way.

@ori-near ori-near merged commit 9f70d41 into master May 15, 2023
5 checks passed
@ori-near ori-near deleted the robert/template branch May 15, 2023 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Housekeeping An update to the NEPs repo organization. S-draft/needs-moderator-review A NEP in the DRAFT stage that needs a moderator review. WG-unknown No Work Group is currently accountable
Projects
Status: APPROVED FIXES
Development

Successfully merging this pull request may close these issues.

None yet

3 participants