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

No clear list of permitted keys in "Declaring project metadata" specification #1101

Closed
JDLH opened this issue Jun 28, 2022 · 6 comments · Fixed by #1168
Closed

No clear list of permitted keys in "Declaring project metadata" specification #1101

JDLH opened this issue Jun 28, 2022 · 6 comments · Fixed by #1168
Labels
component: specifications type: enhancement A self-contained enhancement or new feature

Comments

@JDLH
Copy link

JDLH commented Jun 28, 2022

The Declaring project metadata specification has an important task of listing the field names which are permitted in the [project] table of the pyproject.toml file. (It also has other tasks, such as defining the permitted types for values of those fields, the specific semantics of the key-value entries, and references to other specifications such as PEP508.) However, as I read the document text, it does not contain a clear and straightforward list of those keys.

Observed behaviour: the document defines 10 field names by formatting those strings as monospace (``) and headings (===), for example "readme". The document defines two pairs of field names by formatting those strings as a pair of monospace strings, separated by a non-monospace slash ("/") character, all formatted as a heading (for example, "authors/maintainers"). Unless one reads carefully, it easy to mistake these pairs as a single field name which includes a slash character, i.e. the name authors/maintainers. The document defines three names — actually subtable names rather than field names — by listing them within the text of a line which is labelled as "TOML type" (for example, "scripts").

Desired enhancement: add a section to the document with a list of field names. This could be a bullet point list. It could also be a table, with a row for each permitted field, and columns to give the field name, the TOML type, and perhaps a summary. This section would be located above the sections describing each field's semantics, i.e. before the name section.

Discussion:

I believe this is a "readability improvements that [doesn’t] affect software interoperability", and so it is appropriate to "implement[] using the Python Packaging User Guide’s standard pull request based workflow". I think this means I don't need to write a PEP. Hence this GitHub Issue, which I can follow up with a Pull Request if there is interest.

The two pairs of names, defined in a single section heading, were probably combined for brevity. Their types are identical, and their semantics are very similar. It would also be an improvement to reword those headings, replacing the slash by clearer wording. For instance, change heading text "authors/maintainers" to read "authors and maintainers fields". Better yet, make the existing section be about just the first name of the pair, and add a second section with the second name of the pair, and brief body text saying "this is just like the previous section, but instead of X use Y." This improvement could be added to the scope of this Issue, or saved for a separate Issue.

Three names are actually names of TOML subtables rather than TOML fields. Specifying tables works better with a different structure than what this document uses for specifying fields. Trying to unify the two results in text like, "TOML type: table ([project.scripts], [project.gui-scripts], and [project.entry-points])", where the word "table" doesn't really specify the data type, and the words "([project.scripts], [project.gui-scripts], and [project.entry-points])" are not TOML types. I think this is best solved as part of a rewrite which includes propagating other TOML syntax constraints into this document. For example, this document should specify whether field names are case-sensitive, and whether names may be quoted or not, and whether inline tables and regular tables are equivalent. I suggest that the enhancement to describe this set of TOML syntax details be covered by a different GitHub Issue.

@pradyunsg pradyunsg added type: enhancement A self-contained enhancement or new feature component: specifications labels Jul 11, 2022
@pradyunsg
Copy link
Member

Thanks for filing this issue! Yes, this definitely does not need a PEP. I appreciate that you took the time to go look that up. :)

This seems like a reasonable set of improvements to the page, and please feel welcome to file a PR for this! I'll flag that @CAM-Gerlach has been working on roughtly the same space (although, based on my understanding of #955 (comment), his PR won't be touching this page extensively). He can comment on that. :)

I have a few thoughts on how to go about this -- please treat them as suggestions and not specific requests/demands/guidance:

  • I think having a list of all the specific keys near the top of the page is a reasonable improvement. This can take the form of a .. contents: directive, on the top of the page:

    .. contents:
       :local:
    

    This has the benefit of reusing the existing headings on the page, avoiding duplication of the listing of keys.

  • I'd probably change it to "authors and maintainers", omitting the "fields" -- since there's really no vocabulary for fields in the TOML spec (as far as I remember).

  • For the Entry points heading, I think changing that to something like the following would make sense.

    Entry points (`scripts`, `gui-scripts` and `entry-points`)
    

    They are all closely related, so splitting into separate headings would not be particularly beneficial.

  • The data type for the entry points should probably refer to what exactly the types of the key-value pair is supposed to be ({str: str}, since keys in TOML are always strings), and dropping the bracketed list of table names is also a good improvement.

@CAM-Gerlach
Copy link
Contributor

I'll flag that @CAM-Gerlach has been working on roughly the same space (although, based on my understanding of #955 (comment), his PR won't be touching this page extensively). He can comment on that. :)

Thanks for the heads up. #955 shouldn't really touch PEP 621 at all, aside from linking a few new terms/specs at the beginning and end.

However, after discussion with @brettcannon in python/peps#2680 , I was planning to drop another PR soon adding the examples from the PEP to the spec, along with corresponding examples of the resulting core metadata field value(s) the [project] table value would produce, to greatly aid clarity and to address several points of confusion raised by various users. This would be a more to work around for one or the other of us, but alternatively I make your suggested changes directly in that PR instead, if that's okay with you.

  • I think having a list of all the specific keys near the top of the page is a reasonable improvement. This can take the form of a .. contents: directive, on the top of the page:

I didn't realize they weren't listed in the sidebar in the PyPA theme. If and when the PyPUG switches to @pradyunsg 's Furo and/or Lutra themes, there would be a list of them in the right sidebar anyway, but it could be useful for now. Same would go for the core metadata fields, really.

  • I'd probably change it to "authors and maintainers", omitting the "fields" -- since there's really no vocabulary for fields in the TOML spec (as far as I remember).

I would think you of all people would remember 😆

In fact, the whole spec uses the term "fields" extensively to mean what is correctly termed a "key" (while referring to the table sub-keys as such), which causes significant ambiguity since it also uses that same term (correctly, since it is what its own spec uses) for the core metadata fields. I would suggest switching to the clearer, more consistent and unambiguous terminology used in PEP 639 throughout this spec to avoid this specific issue (which caused me significant confusion when reading it for that PEP), namely always using "key" for [project] keys and "field" for core metadata fields.

  • For the Entry points heading, I think changing that to something like the following would make sense. [...] They are all closely related, so splitting into separate headings would not be particularly beneficial.

Thinking some about it, I think I'd actually lean toward still splitting them for the same reasons as the others, to be clear and consistent and because they do have some individual quirks, so that each top-level heading corresponds to one key—notably, this is currently the case in the Core Metadata spec, even for fields that are quite similar (including these). However, failing that, I'd suggest the wording "entry-points, scripts, gui-scripts (possibly inserting "and" if needed for clarity), as "Entry Points" is rather redundant and it ensures that the heading content is still (parallel) keys.

@pradyunsg
Copy link
Member

pradyunsg commented Jul 15, 2022

clearer, more consistent and unambiguous terminology used in PEP 639

I like that about the rewrite -- and, yea, it's probably a good improvement. It's not strictly something we'd need to do for solving the suggestions made here and -- as is becoming somewhat of a pattern lately -- I wanna push back strongly against scope-creeping the eventual PR for this issue. This can be handled separately, and let's do that. We can also make progress toward that eventual goal, but I wouldn't block the PR for that.

I'd actually lean toward still splitting them [snip] failing that, I'd suggest the wording "entry-points, scripts, gui-scripts"

Fair enough -- I don't feel strongly either way, and will defer to whomsoever files the PR to make the initial call on the specific approach taken for these multi-key headings.

@pradyunsg
Copy link
Member

I'll add a cross reference and note that this is separate from #895. :)

@bhrutledge
Copy link
Contributor

bhrutledge commented Dec 4, 2022

@pradyunsg Sorry, do you think this was closed prematurely?

@pradyunsg
Copy link
Member

No -- I saw this being closed and thought it was that other issue. I added a cross-reference, in case someone else has the same confusion. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: specifications type: enhancement A self-contained enhancement or new feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants