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

Reduce API clutter by only taking String (instead of URI) #147

Closed
ksclarke opened this issue Dec 5, 2021 · 2 comments
Closed

Reduce API clutter by only taking String (instead of URI) #147

ksclarke opened this issue Dec 5, 2021 · 2 comments
Assignees
Labels

Comments

@ksclarke
Copy link
Owner

ksclarke commented Dec 5, 2021

Right now there are convenience/overloaded constructors/methods to do the dance around the fact that our ID strings are URIs.

One alternative would be to accept only strings but do the URI check behind the scenes and throw a runtime exception if the supplied string isn't a URI (the spec does say IDs should be URIs). I'm in favor of a runtime exception instead of a checked exception because if this value is coming from user input it should be checked by the thing that's collecting that user input. If it's coming from the programmer (directly or via another program that generates CSVs, for instance) then passing a bad URI ID is a programming error (hence, the runtime exception).

The other alternative would be to take only the URIs but that's making the programmer jump through hoops (use a URI instead of a string) when most of the time that string value would be a valid URI.

Still thinking about this, but thought I'd jot a note on the current state of my thought.

@ksclarke ksclarke self-assigned this Dec 5, 2021
@ksclarke
Copy link
Owner Author

ksclarke commented Feb 9, 2023

Another argument for String instead of URI is that the spec says all IDs for things in the manifest MUST be HTTPS (except things pointing outside the manifest, like profiles), and this is a level of validation beyond URI alone (so validation still needs to be done in the resource that takes the ID -- in the case of resources that have the requirement).

https://iiif.io/api/presentation/3.0/#id

Going to make this change with #178 since that's also a breaking change (I'm shifting around the location of the annotation classes and changing their hierarchies).

@ksclarke
Copy link
Owner Author

Changes in main but not published to a version yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant