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

Guidelines pre-PEP: add One header, Naming, Language support #51

Merged
merged 7 commits into from
Aug 14, 2024

Conversation

encukou
Copy link
Contributor

@encukou encukou commented Jul 22, 2024

Hello,
Here are a few more proposed sections for the upcoming guidelines PEP.

@zooba @vstinner @erlend-aasland @serhiy-storchaka @mdboom, please add your comments or approvals.

I'd like to avoid growing the scope of this PR. For substantial additions, or things that need more discussion, there's a TODO list in #44.
If a section is controversial I can delete it for now.

Copy link

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

In general LGTM, but I think that the PY_ prefix should be used in some cases.

You can deprecate and remove the old version following Python's
:pep:`backwards compatibility policy <387>`.

After API has been removed, do not reuse the old name,

Choose a reason for hiding this comment

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

It could be helpful to have a list of "retired" names to check for conflicts.

Copy link
Contributor Author

@encukou encukou Jul 25, 2024

Choose a reason for hiding this comment

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

Yes. On the other hand,

  • I don't think it's too likely that someone reuses a name accidentally, and
  • if they do, it's not a big problem. [edit: added forgotten “not”]

The main point is that you shouldn't intentionally retire API to make its name available.

@erlend-aasland
Copy link
Contributor

Looks good; I added a few minor remarks.

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Copy link
Contributor

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM overall, I just left minor comments.

Co-authored-by: Victor Stinner <vstinner@python.org>
Copy link
Contributor

@zooba zooba left a comment

Choose a reason for hiding this comment

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

LGTM. Would like to see a little bit more about the compiler-specific features comment, but otherwise happy to see this merge.

guidelines.rst Outdated
that is, the types of all arguments must be specified.

Argument *names* should generally be included in declarations,
but may be omitted if the meaning is obvious.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we gain anything from this? Might as well just require it, IMHO. And should we require that names match the definition? (I suppose allowing variation saves us from PRs making fixes to our existing inconsistencies...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is purely for humans that read the headers.
More than definitions, I'd care about matching the documentation :)

I don't think we'll be enforcing this, except if someone adds an inscrutable chunk of API. It's easy enough to fix later.
Hence the weasel words.

This document is for new API, I don't plan PRs to fix inconsistencies.

(For definitions: if there's an argument PyObject *x, I'd be OK with the definition calling it x_obj, type-check it, and have PyLongObject *x be the cast version.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't plan PRs to fix inconsistencies.

I know you don't, but we'll get other people who will come along and create PRs to fix things that don't match new guidelines.

Maybe it's best to just say nothing?

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. PEP-7 would be a better place for this, anyway.

guidelines.rst Outdated
that is, the types of all arguments must be specified.

Argument *names* should generally be included in declarations,
but may be omitted if the meaning is obvious.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is purely for humans that read the headers.
More than definitions, I'd care about matching the documentation :)

I don't think we'll be enforcing this, except if someone adds an inscrutable chunk of API. It's easy enough to fix later.
Hence the weasel words.

This document is for new API, I don't plan PRs to fix inconsistencies.

(For definitions: if there's an argument PyObject *x, I'd be OK with the definition calling it x_obj, type-check it, and have PyLongObject *x be the cast version.)

@encukou
Copy link
Contributor Author

encukou commented Aug 12, 2024

Thank you for the reviews, everyone!
I plan to merge on Wednesday, leaving more changes for follow-up PRs.

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.

5 participants