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

Consolidate information from the three RFCs it is based on #11

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

derickr
Copy link
Member

@derickr derickr commented Dec 4, 2024

No description provided.

@derickr
Copy link
Member Author

derickr commented Dec 4, 2024

I've finally started doing some work on consolidating the content in these files - it would be helpful if you could have a look to make sure I didn't change the meaning. The original document used "should" as the RFC 2119 "MUST", which is rather unfortunate. I believe I have conveyed the original meaning though. Comments welcome! Once you're OK too, I'll put up a short RFC to ack/back this PR (and the ones for the other documents).

@Crell @iluuu1994 @Girgias

@derickr derickr requested a review from iluuu1994 December 4, 2024 11:22
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

From my PoV this follows the established guidelines.

or on GitHub.

Extensions may move between these three categories over time. ``hash`` and
``json`` recently moved from "bundled" to "required" (though I believe
Copy link
Contributor

Choose a reason for hiding this comment

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

Recently as of when? The example should fix a release in which the move happened, for clarity.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated that.


Extensions may move between these three categories over time. ``hash`` and
``json`` recently moved from "bundled" to "required" (though I believe
extensions never move out of the "required" category). ``sodium`` and ``ffi``
Copy link
Contributor

Choose a reason for hiding this comment

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

The parenthetical also seems too personal for a policy document.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated that.

@derickr derickr force-pushed the consolidate-coding-standards branch from a104093 to 8b81759 Compare December 11, 2024 12:13
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

Looks good overall

coding-standards-and-naming.rst Outdated Show resolved Hide resolved
coding-standards-and-naming.rst Outdated Show resolved Hide resolved
coding-standards-and-naming.rst Outdated Show resolved Hide resolved
For the Core/standard/spl extensions, the previous considerations on component
subdivision apply. The fact that string and array functions are not namespaced
does not preclude new namespaced components in these extensions.
extension that has historically been procedural, these MAY be namespaced. For
Copy link
Member

Choose a reason for hiding this comment

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

This says MAY, but the original document says should. I don't necessarily disagree, but it diverges from the original meaning.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this line has changed? I only changed may to MAY in it:

extension that has historically been procedural, these may be namespaced. For

vs

extension that has historically been procedural, these MAY be namespaced. For

Copy link
Member

Choose a reason for hiding this comment

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

I was referring to:

Instead, these extensions should be considered as a collection of different components, and should be namespaced according to these.

It's a bit unclear whether this "should" refers to this the "according to" part, or the namespacing itself, i.e. whether we prefer namespaces over a category_ prefix.

Copy link
Member

Choose a reason for hiding this comment

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

But you're right anyway, the sentence was already there. I missed that.

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants