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 packaging guide and repo-review to match spec13 #438

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Carreau
Copy link
Contributor

@Carreau Carreau commented Jun 5, 2024

@Carreau Carreau changed the title Update packaing guide and repo-review to match spec13 Update packaging guide and repo-review to match spec13 Jun 5, 2024
@Carreau Carreau force-pushed the spec13 branch 3 times, most recently from f662ecd to 9e16f26 Compare June 5, 2024 18:18
@@ -241,5 +241,65 @@ def check(pyproject: dict[str, Any]) -> bool:
return "filterwarnings" in options


class PP310(PyProject):
"Tests target is test not test (spec13)"
Copy link
Collaborator

@henryiii henryiii Jun 5, 2024

Choose a reason for hiding this comment

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

Suggested change
"Tests target is test not test (spec13)"
"Tests extra is `tests` not `test` (spec13)"

?



class PP311(PyProject):
"Tests target is `docs not` `doc` (spec13)"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"Tests target is `docs not` `doc` (spec13)"
"Tests target is `docs` not `doc` (SPEC13)"

def check(pyproject: dict[str, Any]) -> bool | None:
"""

docs target should be `docs` not `doc`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
docs target should be `docs` not `doc`
docs extra should be `docs` not `doc`

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, I am not interested in having a check that goes against the official Python packaging specifications, which state test and doc are reserved for this. This means tools (like Hatch) can assume special meanings for these extras. IMO, SPECs are only for standardizing things not already standardized across Python, and so we can't enforce a SPEC over a the official core specification.

https://packaging.python.org/en/latest/specifications/core-metadata/#provides-extra-multiple-use

However, given that [docs] is more popular, and I can't see any PEP source for that recommendation (other than PEPs stating it is the source of truth), I think we could see if it can be changed or removed, in which case such a check is fine.

@Carreau
Copy link
Contributor Author

Carreau commented Jun 5, 2024

Thanks for the review.

Let's wait on the agreement on the spec and I'll update this.

return len([p for p in package.iterdir() if "doc" in p.name]) > 0


class PY004b(General):
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 think a check can end in a letter. I think this could be merged into the PY004 check? Though some projects have multiple docs-* folders, it's important not to error on those. Maybe just making sure /doc doesn't exist is an option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it works :-)

├── PY003 Has a LICENSE* file ✅
├── PY004 Has docs folder ✅
├── PY004b Documentation folder should be `docs` not `doc` ❌
│   Projects must have documentation in a folder called docs not doc

One of my main concern was to not mark a project as "not having docs", and think of a plan to convey to projects that doc used to be ok.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, interesting. I feel like I've encoded this somewhere. Maybe not.

@henryiii
Copy link
Collaborator

henryiii commented Jun 5, 2024

I know there are not many tests yet, but there is a testing system set up if you want to add a couple of tests.

Comment on lines +264 to +271
if "tool" not in pyproject:
return None
if "project.optional-dependencies" not in pyproject["tool"]:
return None
optional_deps = pyproject["tool"]["project.optional-dependencies"]
if "tests" in optional_deps:
return True
return "test" not in optional_deps
Copy link
Collaborator

@henryiii henryiii Jun 5, 2024

Choose a reason for hiding this comment

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

Suggested change
if "tool" not in pyproject:
return None
if "project.optional-dependencies" not in pyproject["tool"]:
return None
optional_deps = pyproject["tool"]["project.optional-dependencies"]
if "tests" in optional_deps:
return True
return "test" not in optional_deps
match pyproject:
case {"tool": "project" { "optional-dependencies": {"tests": _}}}:
return True
case {"tool": "project" { "optional-dependencies": {"test": _}}}:
return False
case _:
return True

Try pattern matching here. ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I need to practice more my pattern matching :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

SPEC 0 should make that easier now. :)

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.

None yet

2 participants