-
Notifications
You must be signed in to change notification settings - Fork 44
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
Make "parameters" singular #836
Conversation
Signed-off-by: Alexios Zavras (zvr) <github@zvr.gr>
Actually... even if we don't change the property name to singular, we should definitely at least change the current text on the property file from:
to something like
since the property is definitely not a map of all key/values pairs; it only holds one. |
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'm fine with adjusting this now, so we don't have a problem down the road, but want to make sure that Nisha & Brandon see it and comment - so we don't have a "surprise"
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 agree with the property name and property description changes proposed here themselves. It makes things more consistent.
as long as its able to encode multiple parameters in the dictionary. I am fine with the labeling change. |
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.
Good catch - I prefer to fix this early. Based on the reply from @lumjjb - I'll assume this is OK to merge.
We should add a line to https://github.com/spdx/spdx-3-model/blob/main/CHANGELOG.md noting this change. @zvr could add the update to this PR or I can add it after merge (since it's getting rather late in Europe) |
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.
Based on Brandon's comments, agree let's update this.
Signed-off-by: Arthit Suriyawongkul <arthit@gmail.com> Signed-off-by: Arthit Suriyawongkul <arthit@gmail.com>
PR #836 missed this change Signed-off-by: Gary O'Neall <gary@sourceauditor.com>
The change log PR is here #839 (Note that I has accidentally make a commit previously. It is already got reverted) |
This makes the property of Build profile
parameter
singular.Whenever we have a "dictionary" or Key-Value pairs, we always use the singular form because in RDF we have multiple such property instances.
Here's a
parameter
with thiskey
and thisvalue
, here's another, and the class instance has all these properties(0..*)
.See, for example https://github.com/spdx/spdx-3-model/blob/main/model/AI/Properties/hyperparameter.md
On the other hand, the plural mirrors the SLSA Provenance definitions, where they use the plural form.
The point is moot, since this is related to SLSA Provenance v0.2 and already v1.0 is out, so presumably we will be updating our model shortly.
We will be having the same discussion about
externalParameters
andinternalParameters
defined there, and whether we keep our decisions or follow conventions of others.So, we can be consistent in our frozen standard version, or follow external conventions, or do one thing now and another when we update... I do not feel very strongly either way, but I wanted to raise this before freezing.