-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Issue 995 - Correctly support version-prerelease+metadata
in fingerprint
#999
Conversation
- DashPy `+` fix - additional test for version-pre+meta case
version-prerelease+metadata
in fingerprintversion-prerelease+metadata
in fingerprint
dash/fingerprint.py
Outdated
@@ -9,7 +9,7 @@ def build_fingerprint(path, version, hash_value): | |||
|
|||
return "{}.v{}m{}.{}".format( | |||
"/".join(path_parts[:-1] + [filename]), | |||
str(version).replace(".", "_"), | |||
str(version).replace(".", "_").replace("+", "__"), |
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.
This works for +
, but how confident are we that +
, -
, and .
are the only characters outside of \w
([A-Za-z0-9_]
) that we're ever going to see? Is there anything to stop people from making a package with
__version__ = "the $%# *best* apple π"
?
and then, to be ultra-paranoid, their next version is:
__version__ = "the $@# *best* apple π"
Or perhaps more likely, some foreign character used as a serial number gets incremented to the next such foreign character.
Feels to me the super-safe way to handle this would be to replace each [^\w-]
character with an escape code, something like "_{:x}".format(ord(char))
. If we want to keep .
as just _
that seems pretty safe, even if there could be a pathological case that breaks it - in fact starting with any unescaped character then replacing it with itself escaped in the next version (including, today, replacing _
with .
) would yield a matching string... but I'd be willing to ignore that possibility.
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.
[0-9a-zA-Z]
and -
are the only allowed characters in the fragments as per semver.
https://semver.org/
Versions need to be non-negative numbers.
Pre-release begins with -
with n-fragments separated by .
Metadata beings with +
with n-fragments seperated by .
Checking out what https://www.python.org/dev/peps/pep-0440/ expects.. turns out they also support version epoch
which would add !
into the mix (https://www.python.org/dev/peps/pep-0440/#id30)
Is there anything to stop people from making a package with "$%# best apple π"
Yes. [0-9a-zA-Z]
and -
is pretty much what's allowed for both npm and pypi semver.
Although.. maybe some other distribution channel does not follow the same underlying norm npm/pypi follow, for some reason. If that happens, we might have to uri encode those strings too 🤷♂
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.
Alright.. let's go full pathological.
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.
Yep, that'll work. If anyone comes back asking us to actually distinguish [^\w-]
characters so they cache independently, the onus is on them to show that this is in the spirit of some common version scheme. 💃
Fixes #995
Manually tested against
The modified version of the
@plotly/webpack-dash-dynamic-import
pluginApp, with both
eager_loading
true / false to trigger async resources from both DashPy and the plugin