GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Update packaging guide and repo-review to match spec13 by Carreau · Pull Request #438 · scientific-python/cookie

Conversation

Carreau commented Jun 5, 2024

Copy link

Copy Markdown

Member

See scientific-python/specs#324

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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 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 force-pushed the spec13 branch from df14aa0 to 476d61d Compare

June 5, 2024 18:20

henryiii reviewed

Jun 5, 2024

View reviewed changes

Comment thread

src/sp_repo_review/checks/pyproject.py



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

henryiii Jun 5, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

henryiii reviewed

Jun 5, 2024

View reviewed changes

Comment thread

src/sp_repo_review/checks/pyproject.py



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

henryiii Jun 5, 2024

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

henryiii reviewed

Jun 5, 2024

View reviewed changes

Comment thread

src/sp_repo_review/checks/pyproject.py

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

		docs target should be `docs` not `doc`

henryiii Jun 5, 2024

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

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`

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

henryiii Jun 5, 2024

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Carreau commented Jun 5, 2024

Copy link

Copy Markdown

Member Author

Thanks for the review.

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

henryiii reviewed

Jun 5, 2024

View reviewed changes

Comment thread

src/sp_repo_review/checks/general.py

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


		class PY004b(General):

henryiii Jun 5, 2024

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Carreau Jun 5, 2024

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

henryiii Jun 5, 2024

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

henryiii commented Jun 5, 2024

Copy link

Copy Markdown

Collaborator

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

henryiii reviewed

Jun 5, 2024

View reviewed changes

Comment thread

src/sp_repo_review/checks/pyproject.py

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

henryiii Jun 5, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

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. ;)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Carreau Jun 5, 2024

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

henryiii Jun 5, 2024

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

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. :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Carreau commented Sep 16, 2024

Copy link

Copy Markdown

Member Author

closing for now to reduce noise in opened PRs.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Carreau closed this

Sep 16, 2024

Reviewers

henryiii

henryiii left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

Carreau commented Jun 5, 2024

Uh oh!

henryiii Jun 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Carreau commented Jun 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henryiii commented Jun 5, 2024

Uh oh!

henryiii Jun 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Carreau commented Sep 16, 2024

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

henryiii Jun 5, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

henryiii Jun 5, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.