-
Notifications
You must be signed in to change notification settings - Fork 13
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
Add implementation of: https://ocfl.github.io/extensions/0006-flat-om… #43
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! I left a few implementation notes. Additionally, there are a couple other places in the code that need updated:
- It should be added to this map
- It should be added to the list of implemented storage layout extensions in the README
.../java/edu/wisc/library/ocfl/core/extension/storage/layout/FlatOmitPrefixLayoutExtension.java
Outdated
Show resolved
Hide resolved
.../java/edu/wisc/library/ocfl/core/extension/storage/layout/FlatOmitPrefixLayoutExtension.java
Outdated
Show resolved
Hide resolved
.../java/edu/wisc/library/ocfl/core/extension/storage/layout/FlatOmitPrefixLayoutExtension.java
Outdated
Show resolved
Hide resolved
.../java/edu/wisc/library/ocfl/core/extension/storage/layout/FlatOmitPrefixLayoutExtension.java
Outdated
Show resolved
Hide resolved
.../java/edu/wisc/library/ocfl/core/extension/storage/layout/FlatOmitPrefixLayoutExtension.java
Outdated
Show resolved
Hide resolved
.../java/edu/wisc/library/ocfl/core/extension/storage/layout/FlatOmitPrefixLayoutExtension.java
Outdated
Show resolved
Hide resolved
.../java/edu/wisc/library/ocfl/core/extension/storage/layout/FlatOmitPrefixLayoutExtension.java
Outdated
Show resolved
Hide resolved
...a/edu/wisc/library/ocfl/core/extension/storage/layout/config/FlatOmitPrefixLayoutConfig.java
Outdated
Show resolved
Hide resolved
...a/edu/wisc/library/ocfl/core/extension/storage/layout/config/FlatOmitPrefixLayoutConfig.java
Outdated
Show resolved
Hide resolved
.../java/edu/wisc/library/ocfl/core/extension/storage/layout/FlatOmitPrefixLayoutExtension.java
Outdated
Show resolved
Hide resolved
Additionally, there should be a copy of the spec here: https://github.com/UW-Madison-Library/ocfl-java/tree/main/ocfl-java-core/src/main/resources/ocfl-specs |
Thanks for the review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I believe it's just missing the registry entry.
public static final String EXTENSION_NAME = "0006-flat-omit-prefix-storage-layout"; | ||
|
||
private FlatOmitPrefixLayoutConfig config; | ||
private boolean isCaseSensitive = true; // The spec does not provide for configuring this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During the init
call you could do something like:
isCaseSensitive = !delimiter.toLowerCase().equals(delimiter.toUpperCase());
Then, if it's not case sensitive, there's no need to lower case the object id on mapObjectId
.
If you don't care about this optimization, then we probably don't need the isCaseSensitive
field.
String delim = config.getDelimiter(); | ||
String id = objectId; | ||
if (isCaseSensitive) { | ||
delim = delim.toLowerCase(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you stick with isCaseSensitive
, then you may was well compute this lower case delimiter value once on init
.
Looks good. Will merge when I'm back from vacation on Monday |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@pwinckles : I will keep my eye our for a release in order to remove the extension logic from our codebase. Thanks! |
Just released it, but it may take a little bit to show up in central. |
…it-prefix-storage-layout.html