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

misspelled getCaseSensisitiveId in SpdxListedLicenseModelStore #266

Open
bact opened this issue Dec 17, 2024 · 9 comments
Open

misspelled getCaseSensisitiveId in SpdxListedLicenseModelStore #266

bact opened this issue Dec 17, 2024 · 9 comments
Milestone

Comments

@bact
Copy link
Collaborator

bact commented Dec 17, 2024

Two misspelling here:

  • getCaseSensisitiveId -> getCaseSensitiveId
  • caseInsensisitiveId -> caseInsensitiveId

@Override
public Optional<String> getCaseSensisitiveId(String documentUri, String caseInsensisitiveId) {
Optional<String> retval = listedLicenseIdCaseSensitive(caseInsensisitiveId);
if (retval.isPresent()) {
return retval;
}
return listedExceptionIdCaseSensitive(caseInsensisitiveId);
}

  • They are from a public method.
  • Changing any of them will break any code that use the misspelled one.
@bact
Copy link
Collaborator Author

bact commented Dec 17, 2024

As SpdxListedLicenseModelStore implements IListedLicenseStore, which extends IModelStore, probably have to look at IModelStore in spdx-java-core:

The PR is a breaking change.

@goneall
Copy link
Member

goneall commented Dec 17, 2024

@bact - I saw this as well - since this is public, I'm not going to fix it until we do another major release - worried it may break some downstream code

@goneall goneall added this to the 3.0 milestone Dec 17, 2024
@pmonks
Copy link
Collaborator

pmonks commented Dec 18, 2024

One option would be to rename these methods to the correct spellings, then add the old spellings back, marked as deprecated and delegating to the methods with the correct spellings.

This has the advantage of being backwards compatible while also giving users the option to migrate to the new methods when they're ready.

@bact
Copy link
Collaborator Author

bact commented Dec 18, 2024

Thank you @pmonks , I think that would work for methods.

For enum, if we can specify the value of two enums to be the same, will it work too?

@pmonks
Copy link
Collaborator

pmonks commented Dec 19, 2024

@bact I don't believe Java allows one Enum to alias another, or have multiple symbols in a single Enum that have the same value, but it's been a while since I've been a regular Java user so I may very well be wrong.

@bact
Copy link
Collaborator Author

bact commented Dec 19, 2024

You're right. Java Enum works at instance level, not value. So it will not work :(

@bact
Copy link
Collaborator Author

bact commented Dec 19, 2024

One option would be to rename these methods to the correct spellings, then add the old spellings back, marked as deprecated and delegating to the methods with the correct spellings.

This has the advantage of being backwards compatible while also giving users the option to migrate to the new methods when they're ready.

Looking at this again, it's a bit complicated than I was thought as IModelStore is an interface. Which means even if we opt for having two methods (one correctly spelled and one spelled the same for backward compat), the code will break anyway because any class that implements IModelStore has to implement both of the methods (but they currently have only one).

@bact
Copy link
Collaborator Author

bact commented Dec 19, 2024

Looking at this again, it's a bit complicated than I was thought as IModelStore is an interface. Which means even if we opt for having two methods (one correctly spelled and one spelled the same for backward compat), the code will break anyway because any class that implements IModelStore has to implement both of the methods (but they currently have only one).

Find probably a solution. Java 8 introduces a "default method" that can solve this issue.
The default method can be implemented directly inside the interface, so any class that implements the interface doesn't have to (they can still override it).
https://docs.oracle.com/javase/tutorial/java/IandI/defaultmethods.html

@pmonks do you mind to see if spdx/spdx-java-core#8 makes sense? Thank you

What I'm not certain is which method should be a default one.

@goneall
Copy link
Member

goneall commented Dec 27, 2024

It looks like the default method approach should work. To be honest, this is the first time I heard about the approach. Let's give spdx/spdx-java-core#8 a try and see if anything downstream breaks.

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

No branches or pull requests

3 participants