Skip to content

Conversation

@dwalluck
Copy link
Contributor

Is something like this possible? It could remove the need to manually call SpdxModelFactory::init.

@dwalluck
Copy link
Contributor Author

dwalluck commented Jan 27, 2025

It appears that the creation of a TypedValue requires the specVersion to already be registered or it throws an exception.

I think that this is the only place in the code where this occurs since the other classes which call new TypedValue(), e.g., Spdx*ListedLicenseModelStore, hold a reference to the agent.

@goneall
Copy link
Member

goneall commented Jan 27, 2025

Is something like this possible? It could remove the need to manually call SpdxModelFactory::init.

I'm not sure if this path will always be taken - it's possible to create an SPDX model without a reference to a license or listed license

I haven't been able to come up with a single entrypoint to hook the init to.

@dwalluck
Copy link
Contributor Author

dwalluck commented Jan 27, 2025

It could still be possible to have it here as a guard to initialize it if it hasn't been initialized already and keep best practices to call it manually. At least for the library code itself, it has to be initialized here or it throws an exception. To put it another way, it has to be initialized by the time we get here, so why not call it rather than fail?

As long as it's not being called too early, it seems harmless to me.

At least in my usage, it removes the need to call it manually, but as you said, depending upon the use case, the caller may need to do it.

@goneall
Copy link
Member

goneall commented Jan 27, 2025

It could still be possible to have it here as a guard to initialize it if it hasn't been initialized already and keep best practices to call it manually. At least for the library code itself, it has to be initialized here or it throws an exception. To put it another way, it has to be initialized by the time we get here, so why not call it rather than fail?

As long as it's not being called too early, it seems harmless to me.

At least in my usage, it removes the need to call it manually, but as you said, depending upon the use case, the caller may need to do it.

@dwalluck - I agree with your logic. We should add a comment to this effect in the code - I'll make a proposal in a review comment.

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.

Proposed comment clarification

Signed-off-by: Gary O'Neall <[email protected]>
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.

LGTM

@goneall
Copy link
Member

goneall commented Jan 28, 2025

I added some documentation in #286

I'll go ahead and merge this PR.

@goneall goneall merged commit fceaa22 into spdx:master Jan 28, 2025
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.

2 participants