Skip to content
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

A canonical way to create ExtractedLicenseInfo (Helper method and documentation, Issue #215) #216

Merged
merged 2 commits into from
Nov 24, 2023

Conversation

jkrae-metaeffekt
Copy link
Contributor

@jkrae-metaeffekt jkrae-metaeffekt commented Nov 10, 2023

Adds a helper class in ModelObject for correct creation of ExtractedLicenseInfo objects. This should resolve #215.

I also added documentation and comments regarding correct use of such objects. These may need correction:

It appeared to me that instances ExtractedLicenseInfo and an SpdxDocument become inherently linked after construction and are only assumed to produce correct behaviour when used together. I tried to document this, since it's a detail critical to data integrity but remains intransparent to me due to the complex workings of *ModelStore.:
https://github.com/jkrae-metaeffekt/Spdx-Java-Library/blob/fbfd9acdc9aa81a2f17c7e5dff101c1fec495412/src/main/java/org/spdx/library/model/ModelObject.java#L1420C2-L1422 .

Please correct me on this. In case of correction, the other added javadocs may need adjustment, too.

edit: formatting fix

…ctly

Signed-off-by: jkraenzke <jan.kraenzke@metaeffekt.com>
Signed-off-by: jkraenzke <jan.kraenzke@metaeffekt.com>
Copy link
Collaborator

@pmonks pmonks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than my trivial comment about imports, this looks great! I'll give @goneall a chance to weigh in too, though I know he's OOO for a few weeks.

What kind of timeline are you hoping to have this change integrated in, @jkrae-metaeffekt? Note that while I (think) I can merge this PR, I don't have the means to cut a new release (and even if I did, I'd want @goneall to approve that first).

@@ -40,13 +40,8 @@
import org.spdx.library.model.enumerations.ReferenceCategory;
import org.spdx.library.model.enumerations.RelationshipType;
import org.spdx.library.model.enumerations.SpdxEnumFactory;
import org.spdx.library.model.license.AnyLicenseInfo;
import org.spdx.library.model.license.ConjunctiveLicenseSet;
import org.spdx.library.model.license.*;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the existing codebase, I think @goneall prefers explicit imports, rather than wildcard imports. FWIW I'd be in favour of using wildcard imports though, if he's open to that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point about keeping style with the imports. Do you reckon I should just change history and force-push this fix?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with the wildcards and the minor style inconsistency in using a mix of specific imports and wildcard. I have my IDE set to generate the specific imports - mainly to avoid possible conflicts but in this case the public classes shouldn't create any conflicts.

@jkrae-metaeffekt
Copy link
Contributor Author

Timeline? Huh. "ASAP" is just about the most useless comment to that question, i guess.

I'm now using the correct constructor as a workaround for now (see #216). So my code works now.
It would undoubtedly be nice to have in the next release, though.

Copy link
Member

@goneall goneall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jkrae-metaeffekt - changes look good.

@goneall goneall merged commit df46822 into spdx:master Nov 24, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

In need of a canonical way to add license texts for licenseRefs / Maybe bugged?
3 participants