-
Notifications
You must be signed in to change notification settings - Fork 147
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
Declare metadata in pyproject.toml, remove hatch-nodejs-version plugin #427
Declare metadata in pyproject.toml, remove hatch-nodejs-version plugin #427
Conversation
17b5038
to
3e4e01a
Compare
Wherever possible, sticking to static fields in PEP 621 whenever possible is going to be a lot more stable over time. I think Somewhat off-topic...`poetry` is _particularly_ bad at the above (and has plugins that won't work as a proper [PEP 517](https://peps.python.org/pep-0517/) `build-system`), so three cheers for this project not using that!On the main, I've found simple and dumb is better at the actual build-an-archive stage... but the lab community has seemed to gel around the sandwich of a python build system building a huge virtual environment, calling Indeed, I actually prefer the anti-extensible |
7057e0d
to
1234bab
Compare
Test failures assumed to be unrelatedWe have test errors now, but I don't think I broke it in this PR. This PR is on the other hand fixing our test suite in other regards so I suggest it gets merged to enable easier focus on resolving the other test failure about "socket already in use" |
Thank you @bollwyvl for reviewing this!!! Sorry for the slow followup - ready for review again! |
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.
Seems OK to me, but I'll let @bollwyvl make the final decision
@@ -1,5 +1,6 @@ | |||
from jupyter_server.utils import url_path_join as ujoin | |||
|
|||
from ._version import __version__ # noqa |
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.
Perhaps add an __all__
so this doesn't have to be noqa
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.
Would you like the __all__
list everythign currently exported or only some attributes (possibly breaking)?
Ideally, we'd have __all__
listing only what was needed to be exported historically, but I'm not confident on constraining what we export at this point.
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 guess once something has been hoisted to __init__.py
, for whatever reason, it is part of the public API. Doing in-the-wild searching sounds more tiresome.
This might be an appropriate time to consider getting on the recent push to get more of the stack working with mypy
, with a backstop of the unused-imports
rules from ruff
, which can more generally be informed by sp-repo-review. This is likely better than making on-the-fly, case-by-case calls on specific repos.
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.
That sounds interesting @bollwyvl! I'm not able to drive that effort as part of this PR though, feeling somewhat overloaded atm :/
@bollwyvl I'll go for a merge at this point, I hope that is considered acceptable. I'd like to see us get the projects CI system back and functional and this is just one step along the way it seems. |
When I made a release I bumped to a dev version after, but that had
pyproject-build
start to fail. This was because thehatch-nodejs-version
plugin didn't allow us to use a version in package.json that was4.1.1-0.dev
which is a valid semver2 version. In helm chart's etc that require semver2 compliant versions like package.json also wants, we've ended up using the-0.dev
version suffix as it allows us to sort that version before alpha if we make something like that sometime.In this PR I've instead of adjusting the version 4.1.1-0.dev to something that worked with
hatch-nodejs-version
plugin adjusted to no longer use it. I've as part of this removed some metadata from package.json and put it directly in pyproject.toml. The version field wasn't put directly there, but in_version.py
, allowing it to be inspected via an import statement by other packages.