Search code, repositories, users, issues, pull requests...

Copy link

Contributor

@Carreau Carreau commented Jun 5, 2024

See scientific-python/specs#324

@Carreau Carreau changed the title ~~(削除) Update packaing guide and repo-review to match spec13 (削除ここまで)~~ (追記) Update packaging guide and repo-review to match spec13 (追記ここまで)

@Carreau Carreau force-pushed the spec13 branch 3 times, most recently from f662ecd to 9e16f26 Compare

June 5, 2024 18:18


 Update packaing guide and repo-review to match spec13

476d61d

See scientific-python/specs#324

@Carreau Carreau force-pushed the spec13 branch from df14aa0 to 476d61d Compare

June 5, 2024 18:20

henryiii

henryiii reviewed

class PP310(PyProject):

"Tests target is test not test (spec13)"

Copy link

Collaborator

@henryiii henryiii Jun 5, 2024 •

edited

Loading

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)"

henryiii

henryiii reviewed

class PP311(PyProject):

"Tests target is `docs not` `doc` (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 `docs not` `doc` (spec13)"

"Tests target is `docs` not `doc` (SPEC13)"

henryiii

henryiii reviewed

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

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

"""

docs target should be `docs` not `doc`

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

docs target should be `docs` not `doc`

docs extra should be `docs` not `doc`

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.

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.

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.

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.

henryiii

henryiii reviewed

src/sp_repo_review/checks/general.py

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

class PY004b(General):

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.

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

@Carreau Carreau Jun 5, 2024

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

@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.

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.

henryiii

henryiii reviewed

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 •

edited

Loading

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

@Carreau Carreau Jun 5, 2024

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

@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.

SPEC 0 should make that easier now. :)