-
Notifications
You must be signed in to change notification settings - Fork 29
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
Add key
and id
properties to ArtefactMetadata
model class
#1089
base: master
Are you sure you want to change the base?
Conversation
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.
lgtm. will leave addressing of my comments up to you
def as_frozenset(self) -> frozenset[str]: | ||
props = ( | ||
@property | ||
def key(self) -> str: |
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.
since key is quite imporant, and its intended use might not be obvious, I think a short docstr might be adequate (state, in particular, that key should identify this artefact-metadata uniquely, and is intended to be used as pk in underlying db)
@@ -13,6 +14,14 @@ | |||
import unixutil.model | |||
|
|||
|
|||
def _as_key( |
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.
key + _as_key might be candidates for being factored into a mixin
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.
as to my second set of comments (w.r.t. unittest / guarding against changes: please consider that one)
@property | ||
def id(self) -> str: | ||
return hashlib.sha1(self.key.encode('utf-8'), usedforsecurity=False).hexdigest() | ||
|
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.
as discussed recently in a different context: how about using a shorted digest + how about base64-encoding it (as opposed to hex-str) so save some more chars?
regardless: I think we should safeguard ourselves from accidentally changing this implementation via a unittest (with a comment explaining the implications of changing id-calculation)
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Release note: