Create concrete classes for license individuals #588

goneall · 2023-12-23T19:56:29Z

Fixes #574

This PR implements concrete classes by the same name as the individuals. The concrete classes inherit from a comment IndividualLicense class.

An alternative solution to #574 is to have the individuals use the IndividualLicense type directly.

The only reason I chose this approach is that having different types may match some language implementations more closely - but I could easily be persuaded to take the alternative solution.

Fixes #574 Signed-off-by: Gary O'Neall <[email protected]>

swinslow · 2024-01-09T20:17:29Z

@goneall Thank you for this! Taking a look now and I will circle back.

swinslow

@goneall Thanks again for this!

I don't have a particular preference for whether to take the approach you described here, or the alternate approach you mentioned of having the individuals NoneLicense / NoAssertionLicense use IndividualLicense directly.

I'll defer to others if there are strong feelings on that point, but since there haven't been any comments here for the past couple of weeks, I'm inclined to go with what you proposed here.

Assuming we take this approach, I have a few comments -- please see below and let me know what you think!

swinslow · 2024-01-09T21:08:06Z

model/ExpandedLicensing/Classes/IndividualLicense.md

+
+## Summary
+
+A concrete subclass of AnyLicenseInfo used by Individuals in the ExpandedLicensing profile.


See my overall comment about whether to use the approach described here or the alternative you mentioned in the discussion thread. Assuming we use the one you did here, though, then is it accurate that IndividualLicense is a "concrete" subclass? Or is it an abstract subclass, since only the NoneLicense and NoAssertionLicense classes should actually be instantiated?

Thanks @swinslow - really good catches.

After reviewing the comments, I no longer see a need for the IndividualLicense class, I can just subclass AnyLicenseInfo directly for None and NoAssertion license types.

I'll update the PR accordingly.

swinslow · 2024-01-09T21:08:14Z

model/ExpandedLicensing/Classes/IndividualLicense.md

+
+- name: IndividualLicense
+- SubclassOf: /SimpleLicensing/AnyLicenseInfo
+- Instantiability: Concrete


Same question / comment as above

swinslow · 2024-01-09T21:09:22Z

model/ExpandedLicensing/Classes/NoAssertionLicense.md

+
+## Summary
+
+A License when no assertion can be made about its actual value.


To align with the NoneLicense class text below, should this say something like:

"A concrete subclass of IndividualLicense used by the NoAssertionLicense Individual in the ExpandedLicensing profile."

I'll align with the text below.

swinslow · 2024-01-09T21:09:58Z

model/ExpandedLicensing/Classes/NoneLicense.md

+
+## Summary
+
+A concrete subclass of AnyLicenseInfo used by Individuals in the ExpandedLicensing profile.


Should this reference IndividualLicense instead of AnyLicenseInfo? And also specify which Individual uses it? E.g.:

"A concrete subclass of IndividualLicense used by the NoneLicense Individual in the ExpandedLicensing profile."

We'll solve this by getting rid of IndividualLicense

Per review comments, I'm removing the IndividualLicense to resolve inconsistencies in the previous commit.

zvr

I don't agree with creating two new classes, with the exact same name as the individuals(!). People will confuse them, they might use the class instead of the individual, etc. It will become messy very quickly,..

And having these classes does not really help. They are concrete, so... can I use them to create more instances/individuals? I shouldn't be able to do so.

I think this approach creates more problems than it solves.
What is the problem we're trying to solve?

goneall · 2024-01-11T15:57:39Z

@zvr - Issue #574 - If you can come up with a better solution, please create a different PR and I'll close this one.

sbarnum · 2024-01-17T16:29:46Z

I agree with @zvr that this approach seems overly complicated and unlikely to lead to the targeted goal.

It seems to me that the simple solution to the issue identified in #574 is simply to make NoAssertionLicense and NoneLicense to be individuals of the LicenseExpression class which is an already existing concrete subclass of AnyLicenseInfo. You could simply have licenseExpression="NONE" or "NOASSERTION" respectively for each Individual.

In the definition of the licenseExpression property on the LicenseExpression class it states "A license expression could be a single license identifier found on the SPDX License List; a user defined license reference denoted by the LicenseRef-idString; a license identifier combined with an SPDX exception; or some combination of license identifiers, license references and exceptions constructed using a small set of defined operators (e.g., AND, OR, WITH and +)."
This should cover the NoAssertionLicense and NoneLicense shouldn't it?

NOTE: Individuals are simply instance objects. They are asserted to be members of one or more classes and they MUST have an globally unique ID (in our case an spdxId). This means that in the SPDX spec where we specify Individuals we need to be explicit that the "name" specified for the Individual is actually its spdxId and those names need to explicitly include the base SPDX id prefix for SPDX defined Individuals (i.e., something more like "https://ontology.spdx.org/3.0/licenses/individuals/NoAssertionLicense" rather than just "NoAssertionLicense").
It seems like it would make sense to keep the "name" field in the markdown spec for an Individual and simply add a new "Id" field with the full Id that the then the value of the "Id" field could be used to generate the id of the formal rdf/owl Individual.

In a nutshell, the only changes required in a PR would be:

In model/ExpandedLicensing/Individuals/NoAssertionLicense.md
1.1 modify from "type: /SimpleLicensing/AnyLicenseInfo" to "type: /SimpleLicensing/LicenseExpression"
1.2 add "id: "NoAssertionLicense""
1.3 under "Property Values" modify from "name: "NOASSERTION"" to "licenseExpression: "NOASSERTION""
In model/ExpandedLicensing/Individuals/NoneLicense.md
2.1 modify from "type: /SimpleLicensing/AnyLicenseInfo" to "type: /SimpleLicensing/LicenseExpression"
2.2 add "id: "NoneLicense""
2.3 under "Property Values" modify from "name: "NONE"" to "licenseExpression: "NONE""

goneall · 2024-01-23T18:19:12Z

model/ExpandedLicensing/Classes/IndividualLicense.md

@@ -1,21 +0,0 @@
-SPDX-License-Identifier: Community-Spec-1.0
-
-# IndividualLicense


IndividualLicenseInfo instead of IndividualLicense

Restore this class

IndividualLicensingInfo... since it's not info on a license

goneall · 2024-01-23T18:27:23Z

Based on tech call on 23 Jan 2023:

Restore the "IndividualLicense" but rename it "IndividualLicensingInfo"
Remove the NoneLicense and NoAssertion license classes from this PR
Change the individuals to be of the type IndividualLicenseingInfo
Have the IDs "https://rdf.spdx.org/v3/Licensing/None
https://rdf.spdx.org/v3/Licensing/NoAssertion"
Have a name "None" and "NoAssertion"

Updates the individuals for license per review comments Signed-off-by: Gary O'Neall <[email protected]>

goneall · 2024-01-24T22:07:37Z

@zvr @sbarnum I just updated the PR with the changes we discussed.

I used the names "NONE" and "NOASSERTION" to be compatible with the previous names used in SPDX 2.X.

For the ID's, I didn't include anything in the markdown file. I figure we would generate the correct ID based on the individual name and profile rather than specify it in the file.

maxhbr

Sorry, I am late to this PR and from reading the diff I can not understand why there are files .../Classes/NoAssertionLicense.md and .../Individuals/NoAssertionLicense.md, but seemingly no connection between them.

Please ignore my question, if that is clear for everyone else.

maxhbr · 2024-01-25T07:27:41Z

model/ExpandedLicensing/Individuals/NoAssertionLicense.md

@@ -15,7 +15,7 @@ the SPDX creator has intentionally provided no information (no meaning should be
 ## Metadata

 - name: NoAssertionLicense
- type: /SimpleLicensing/AnyLicenseInfo
+- type: IndividualLicensingInfo


why is model/ExpandedLicensing/Individuals/NoAssertionLicense.md not of type model/ExpandedLicensing/Classes/NoAssertionLicense.md? I am confused to see it in Individuals and Classes an the same time

maxhbr · 2024-01-25T07:30:02Z

model/ExpandedLicensing/Classes/NoAssertionLicense.md

+## Metadata
+
+- name: NoAssertionLicense
+- SubclassOf: /SimpleLicensing/AnyLicenseInfo


Why is model/ExpandedLicensing/Classes/NoAssertionLicense.md not a subclass of model/ExpandedLicensing/Classes/IndividualLicensingInfo.md, which is "A concrete subclass of AnyLicenseInfo used by Individuals in the ExpandedLicensing profile."

This file should be deleted.

zvr · 2024-01-25T09:28:40Z

You are correct, @maxhbr . @goneall simply forgot to delete the two classes (None and NoAssertion).

The idea is to have only the new class IndividualLicensingInfo (concrete, from SimpleLicensing/AnyLicenseInfo) and the two individuals as instances of that class.

zvr

If the two class files are deleted, this is good to go.

zvr · 2024-01-25T09:29:20Z

model/ExpandedLicensing/Classes/NoAssertionLicense.md

+## Metadata
+
+- name: NoAssertionLicense
+- SubclassOf: /SimpleLicensing/AnyLicenseInfo


This file should be deleted.

zvr · 2024-01-25T09:29:50Z

model/ExpandedLicensing/Classes/NoneLicense.md

+
+- name: NoneLicense
+- SubclassOf: /SimpleLicensing/AnyLicenseInfo
+- Instantiability: Concrete


This file should be deleted.

Signed-off-by: Gary O'Neall <[email protected]>

goneall · 2024-01-25T17:57:38Z

If the two class files are deleted, this is good to go.

Good catch @zvr - The two files are now deleted

sbarnum · 2024-01-30T15:25:26Z

@zvr @sbarnum I just updated the PR with the changes we discussed.

I used the names "NONE" and "NOASSERTION" to be compatible with the previous names used in SPDX 2.X.

For the ID's, I didn't include anything in the markdown file. I figure we would generate the correct ID based on the individual name and profile rather than specify it in the file.

There are only two properties that ANY Individual in ANY ontology MUST define: ID and TYPE.
They MAY define other properties but only have to based on property cardinality restrictions on the TYPE Class.

I strongly believe that the markdown version of the spec needs to include the IDs for ANY Individuals including these two.
When dealing with an Individual the ID is the actual identifier/name for that object.
The 'name' property is only a label.

The intent of the markdown version of the spec is for human readability and clarity.
Anyone who wants to reference either of these Individuals in actual SPDX 3.X content MUST use the ID not the 'name'.
Any code written to deal with the concepts of NONE and NOASSERTION MUST use the ID not the 'name'.

I believe including a 'name' but not the one and only one ID to use for each Individual is very highly likely to lead to confusion and invalid content.

sbarnum

I believe that the IDs for each Individual needs to be included in their respective markdown definition files.
Explanation is in a comment in the Conversation for this PR.

Everything else looks great.

goneall · 2024-01-30T16:56:07Z

I believe that the IDs for each Individual needs to be included in their respective markdown definition files. Explanation is in a comment in the Conversation for this PR.

Everything else looks great.

I don't feel strongly, but I believe the ID's can be assigned by convention in the same way we assign ID's to the class and properties without explicitly stating them in the markdown.

Probably worth a 10 minute discussion with @sbarnum @zvr myself and anyone else interested in the RDF IDs.

One issue for RC2 is that adding an ID field may break the spec-parser. I'll create a separate issue to track fixing this either before RC2 if there is time, or after RC2 and before final release.

zvr · 2024-01-30T21:28:36Z

@sbarnum yes, the individuals must have an id (and they will, in the generated docs and RDF ontology).

@goneall yes, if you put it right now it will break the spec-parser. I will make and push the change on Tue 13 Feb at the latest.

I propose we merge this one and we know we have to add the IRIs at a later stage. If you both remember, I had proposed to use https://rdf.spdx.org/v3/Licensing/None and .../NoAssertion (or something like that, I will check my notes once I'm back).

goneall · 2024-01-31T10:37:29Z

I'll go ahead and merge - we have another issue to track the creation of the IDs.

goneall requested review from zvr and sbarnum December 23, 2023 19:56

goneall added the Profile:Licensing Licensing Profile and related matters label Dec 23, 2023

goneall added this to the 3.0-rc2 milestone Dec 23, 2023

Create concrete classes for license individuals

4970f14

Fixes #574 Signed-off-by: Gary O'Neall <[email protected]>

goneall force-pushed the issue574 branch from 15693a8 to 4970f14 Compare December 23, 2023 20:15

swinslow mentioned this pull request Jan 9, 2024

Update text for SimpleLicensing profile #589

Merged

swinslow reviewed Jan 9, 2024

View reviewed changes

swinslow mentioned this pull request Jan 9, 2024

ExpandedLicensing profile/namespace specifies two individuals (NoAssertionLicense and NoneLIcense) as of type AnyLicenseInfo which is invalid since AnyLIcenseInfo is abstract #574

Closed

Remove IndividualLicense

efc248e

Per review comments, I'm removing the IndividualLicense to resolve inconsistencies in the previous commit.

zvr requested changes Jan 11, 2024

View reviewed changes

goneall commented Jan 23, 2024

View reviewed changes

Add IndividualLicensingInfo

7129aab

Updates the individuals for license per review comments Signed-off-by: Gary O'Neall <[email protected]>

goneall requested a review from zvr January 24, 2024 22:07

maxhbr reviewed Jan 25, 2024

View reviewed changes

zvr requested changes Jan 25, 2024

View reviewed changes

Remove the unused NoAssertionLicense and NoneLicense classes

432a845

Signed-off-by: Gary O'Neall <[email protected]>

zvr approved these changes Jan 25, 2024

View reviewed changes

goneall mentioned this pull request Jan 30, 2024

3.0-RC2 punchlist #622

Closed

32 tasks

sbarnum requested changes Jan 30, 2024

View reviewed changes

goneall mentioned this pull request Jan 30, 2024

Resolve how IDs are handled for individuals #624

Closed

goneall merged commit 3f7f4e8 into main Jan 31, 2024
0 of 2 checks passed

goneall deleted the issue574 branch January 31, 2024 10:37

goneall mentioned this pull request Jan 31, 2024

Update to.md to reflect what min:0 means #623

Merged

zvr mentioned this pull request Feb 13, 2024

Adds IRIs to individual licenses #634

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create concrete classes for license individuals #588

Create concrete classes for license individuals #588

goneall commented Dec 23, 2023

swinslow commented Jan 9, 2024

swinslow left a comment

swinslow Jan 9, 2024

goneall Jan 9, 2024

swinslow Jan 9, 2024

swinslow Jan 9, 2024

goneall Jan 9, 2024

swinslow Jan 9, 2024

goneall Jan 9, 2024

zvr left a comment •

edited

Loading

goneall commented Jan 11, 2024

sbarnum commented Jan 17, 2024 •

edited

Loading

goneall Jan 23, 2024

zvr Jan 23, 2024

goneall commented Jan 23, 2024

goneall commented Jan 24, 2024

maxhbr left a comment

maxhbr Jan 25, 2024

maxhbr Jan 25, 2024

zvr Jan 25, 2024

zvr commented Jan 25, 2024

zvr left a comment

zvr Jan 25, 2024

zvr Jan 25, 2024

goneall commented Jan 25, 2024

sbarnum commented Jan 30, 2024

sbarnum left a comment

goneall commented Jan 30, 2024

zvr commented Jan 30, 2024

goneall commented Jan 31, 2024


		## Summary

		A concrete subclass of AnyLicenseInfo used by Individuals in the ExpandedLicensing profile.


		## Summary

		A License when no assertion can be made about its actual value.

		@@ -1,21 +0,0 @@
		SPDX-License-Identifier: Community-Spec-1.0

		# IndividualLicense

Create concrete classes for license individuals #588

Create concrete classes for license individuals #588

Conversation

goneall commented Dec 23, 2023

swinslow commented Jan 9, 2024

swinslow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zvr left a comment • edited Loading

Choose a reason for hiding this comment

goneall commented Jan 11, 2024

sbarnum commented Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goneall commented Jan 23, 2024

goneall commented Jan 24, 2024

maxhbr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zvr commented Jan 25, 2024

zvr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goneall commented Jan 25, 2024

sbarnum commented Jan 30, 2024

sbarnum left a comment

Choose a reason for hiding this comment

goneall commented Jan 30, 2024

zvr commented Jan 30, 2024

goneall commented Jan 31, 2024

zvr left a comment •

edited

Loading

sbarnum commented Jan 17, 2024 •

edited

Loading