add support for navPlace element #86
Conversation
| def coord_for(long_str, lat_str) | ||
| long_matcher = long_str.match(COORD_REGEX) | ||
| lat_matcher = lat_str.match(COORD_REGEX) | ||
| return unless long_matcher && lat_matcher |
There was a problem hiding this comment.
Do we want to silently fail here, or would an argument exception make it clear that what you are doing is not valid?
There was a problem hiding this comment.
Not sure if I am the correct person to answer this, as this was just a refactor PR to move existing code in PURL to here....but my guess is that by silently failing, we prevent the method from blowing up if there is geo data (in the MODs) but it is not formatted correctly. If we throw an exception, in these cases I'd think we would hit errors in PURL until the data is remediated? Or perhaps there can be multiple formats of geo data in MODs and we only handle one case here?
this is the path where the data is coming from: https://github.com/sul-dlss/purl/pull/821/files#diff-01029093f0ab5a19645c20b7e361bc54e695ff29144aacf226a61bca8dfdd554R353
There was a problem hiding this comment.
I think you ought to check if you have usable data before calling this method. I think this should let you know when you give it the wrong data. Otherwise it becomes a mystery you have to track down.
There was a problem hiding this comment.
Suggestion:
- Add a new method to this class to validate coordinates -- it returns true/false
- Caller (currently only PURL), calls this first and then only calls
buildif valid, otherwise noop (or if you really want to raise in PURL, we can do that too, but I will note that is not what is happening in production at the moment and for this refactor work, I'd suggest preserving functionality as is). buildmethod raises an exception if invalid coordinates are sent in anyway.
Is that what you are thinking
There was a problem hiding this comment.
I'm only thinking of 3 for this gem. You, as the user of the gem can determine how you want to ensure what you're passing in is "good". That could be validating the data before you store it, or filtering out the data you have before you call this method.
If doing 1, also helps you acchieve step 3, then go for it. It's probably better that there is a separate method that validates anyways.
There was a problem hiding this comment.
Did both 1 & 3 (added a .valid? method and used it to determine if we raise an exception on .build?). PURL can use this .valid? method to keep current functionality as is.
There was a problem hiding this comment.
Should the validate method be checking the COORD_REGEX though?
There was a problem hiding this comment.
Nevermind, I see it does that.
peetucket
force-pushed
the
add-nav-place-support
branch
from
849309c
to
7c98d95
Compare
|
Can you merge and then bump version/cut release? I don't have permissions on this repo to do that. I can then move to the PURL changes. |
Fixes sul-dlss/purl#761 : moves
navPlacegeneration from PURL (added here: sul-dlss/purl#752) to theiiif-presentationgem, as well as removes knowledge of public-xml from the class that generates it (and instead leaves that knowledge in PURL).Will be paired with a PURL PR to remove this
NavPlaceclass and replace with a call to this class.