-
-
Notifications
You must be signed in to change notification settings - Fork 7
Add option to canonicalise the version #18
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
base: master
Are you sure you want to change the base?
Add option to canonicalise the version #18
Conversation
Should make this option |
@@ -9,18 +9,34 @@ | |||
|
|||
GOOD_NODE_PYTHON_VERSIONS = [ | |||
("1.4.5", "1.4.5"), | |||
("1.4.5-a0", "1.4.5a0"), |
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.
You're right - a0
is parsed by semver as the pre-release named "a0", not "alpha" version "0". Good catch!
In fact, this shouldn't successfully parse. Can you change the NodeJS REGEX to make the .
required if a pre-release numeral is given? i.e. (probably not this, but approximately)
(?P<pre_n>\.[0-9]+)?
This would mean that pre_n
includes the delimiter in the match results.
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.
The current regex is
(?P<pre_l>(a|b|c|rc|alpha|beta|pre|preview))
[-\.]?
(?P<pre_n>[0-9]+)?
Is it ok to drop the dash? d2139b3 assumes so.
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.
To be explicit, these changes are to the general regex, rather than just the canonicalisation logic you're proposing.
- For Python's sake, I think we probably need to enforce a digit here, because semver treats
1.0.0-a < 1.0.0-a0
. This means that these two versions are distinct for Node.js, but degenerate for Python. - We need to drop the
-
because it would be considered part of the pre-release identifier, i.e.a|b|c|rc|alpha|beta|pre|preview
, rather than an actual separator
I suppose the new pattern would be
(?P<pre_l>(a|b|c|rc|alpha|beta|pre|preview))
\.
(?P<pre_n>[0-9]+)
I'm using
for reference, but they're complex documents, so it's possible I will make mistakes. Let me know if you disagree!
I'd like to, yes. We already do not provide a guarantee of round-trip (unless you stick to a very specific scheme), and so it would be sensible to normalise in all cases. But, most of the users of this package probably don't have any bounds on it, so we would need to be careful not to break existing usage. @henryiii could use your opinion here, because this is a wider question about build backends. We have two options for better compatibility of newer build backends with existing sdists in the wild:
The former is easiest, and feels pretty natural. We'd encourage all users to set The latter means we can give newer code to more people, but I'm not sure if there's much merit. In any case, right now we're fairly open. |
I'd generally avoid asking users to pin. If you do, you should commit to releasing v1 after you release v2 for as long as packages are still around pinning to v1. Build backends are fairly likely to need fixes as Python, OSs, etc. all evolve. Pinning it isn't something you can undo, so if you are stuck on a broken version, you are stuck. The Pythonic way to handle this is have a deprecation period & a warning. That doesn't mean users can't pin, but asking them to pin IMO is promising that pin will be supported in the future. |
@henryiii my worry is about whether we should make it feasible for old sdists to build on newer platforms, especially given how much activity there is in the build backend space (and plugin systems thereof) at the moment. |
Co-authored-by: Angus Hollands <[email protected]>
Minor review suggestions
Thanks for the quick review @agoose77 |
@fcollonval hmm, actually, my comments about our new REGEX may need revising Let me reset my thinking. The challenge here is Node.js and Python handle version strings differently. Originally, I tried to be helpful and map reasonably between the two, but in practice is subjective, and perhaps not as predictable as I'd like. Instead, I think we want to be able to guarantee that we have a one-to-one mapping for the limited scheme that we accept. This means that we could never accept all of
As they all map onto However, such a change would probably break existing users. I would want to make a 1.0.0 release to do this. Because we're already used by lots of packages, I'm not sure if first we would want to introduce a deprecation warning for this. Your thoughts are definitely welcome! |
I would take a pragmatic approach here. Although as you mentioned they are all different versions for Node.js. We can assume that a developer is consistent in its format. So there is a low chance it will change the version format in a way that does not change the Python version. Moreover, if we advice in the documentation to prefer using the CLI
|
I just wanted to drop a line that things are crazy busy right now. I will try and look at this again soon. If this starts to block any Jupyter work, let me know, and I can either bump it in priority, or just bow out of the review. |
Friendly ping to agoose77 |
I remembered that this is still open today. I'll take another look at it soon, and figure out how best to proceed :) |
I'm experiment with source from this repo (ipylab). When I run
It currently produces the following: package.json
_version.py
Notice that the Python version is missing Is the pull request likely to fix this |
@fleming79 could you specify what you'd like to happen here? Python's pre-release segments should not be hyphenated (https://peps.python.org/pep-0440/#pre-releases). |
Is it possible to make them identical? |
@fleming79 I'm pretty sure the answer is no — the Python (PEP440) and SemVer specifications are incompatible. We support round-tripping by parsing PEP440 versions with a different pattern to SemVer. |
@agoose77 . Thank you for considering and the quick response. I guess in this case I could replace |
It is not needed as the package.json is copied within the Python package (the file is needed for lab extension by JupyterLab). In particular for your case, you can find it at To answer more specifically your need, you can get the JS version in the import json
from pathlib import Path
js_version = None
try:
HERE = Path(__file__).parent.resolve()
with (HERE / "labextension" / "package.json").open() as fid:
data = json.load(fid)
js_version = data["version"]
except:
... |
I guess that works for dev, but the distribution puts that data somewhere totally different.
See also a screenshot of the installed package (front right) and the unzipped wheel (back left). It'd be more reliable if it was simply accessible by doing: from ipylab._version import __version__, __javascript_version__ Thank you anyway for the suggestions. In the interim I'll just use: module_version = f"^{__version__}".replace("r", "-r") |
@fleming79 thanks to you and @fcollonval, I now understand the potential use case for this. However, you should be able to use Hatch itself to do this. By default, hatch-jupyter-builder (IIRC) bundles all JS assets under Note that this works because we're asking Hatch to include a file twice, once as part of the Python "package", and once as part of the So, I'd suggest asking Hatch to include |
Thank you both for your assistance.
|
@fleming79 fab! For the avoidance of doubt, I made a typo in my original reply — you both helped me to figure out what we should do here, it's appreciated. |
@agoose77 what should we do with this one? Maybe related to it, would you agree to transfer the owner ship of this to a oss community - like JupyterLab? |
@fcollonval I need to circle back on this and think about whether I still hold the same opinion. Independently of that, I think we should move this to Jupyter stewardship because it's so widely used now; I really don't like that it's a single point of failure, even though I'm no longer the sole maintainer / owner! :) |
Context
Fixes #16
Code changes
For all cases, this adds a
.
betweenpre_l
andpre_n
for nodeJS version; this is recommended - see for example the behavior of.inc
semver.The user can also set a configuration flag
canonical
toTrue
to convert Python package prerelease string toalpha
,beta
orrc
in NodeJS (fallback to using the same string).