Only store largest height, width for characterization, fixes #327

samvera / hydra-works

A ruby gem implementation of the PCDM Works domain model based on the Samvera software stack

Other

24 stars 14 forks source link

Only store largest height, width for characterization, fixes #327 #330

Closed hackartisan closed 7 years ago

hackartisan commented 7 years ago

This can be reviewed; please don't merge until i've run it by the tech list as promised on this week's call. Update: this is ready for review / merge.

coveralls commented 7 years ago

Coverage increased (+0.003%) to 97.956% when pulling 7e8ac6bf2338a0a9fa0f050229bc81f2fbdaa22b on single-val-dimensions into b52a43f73ecc7ada8495ef55c77ae56abf58b6bb on master.

coveralls commented 7 years ago

Coverage increased (+0.003%) to 97.956% when pulling 7e8ac6bf2338a0a9fa0f050229bc81f2fbdaa22b on single-val-dimensions into b52a43f73ecc7ada8495ef55c77ae56abf58b6bb on master.

carolyncole commented 7 years ago

@hackmastera my only concern with this implementation is that you may or may not be matching up the height and width. Is it possible for the arrays to be height: [5,25] & width: [7,2], so you would end up with height:25 width:7 ? I'm guessing that may never happen, but I thought I would point it out...

hackartisan commented 7 years ago

@cam156 I'm not sure that such a file exists. however, if it did, I would probably argue that its cumulative dimensions are indeed 7 x 25.

coveralls commented 7 years ago

Coverage increased (+0.003%) to 97.956% when pulling 7e8ac6bf2338a0a9fa0f050229bc81f2fbdaa22b on single-val-dimensions into b52a43f73ecc7ada8495ef55c77ae56abf58b6bb on master.

hackartisan commented 7 years ago

no idea what this failure means and I'm not getting it locally. @samvera/hyrax-code-reviewers?

mjgiarlo commented 7 years ago

@hackmastera hmm, can you confirm that you've got AF 11.4.0 locally?

The error in Travis is thrown when calling #uri on an AF object that returns true when you ask it if it's a #new_record?. AFAICT, that particular code (in the contains association) hasn't changed in 2-3 years, but what has changed somewhat recently is the #new_record? determination: https://github.com/samvera/active_fedora/compare/v11.2.0...v11.4.0

hackartisan commented 7 years ago

@mjgiarlo good catch! I was running on 11.3.1; didn't update my bundle in the weeks between submitting this PR and coming back around to it. i'm getting the failures now after updating.

It looks like changing these specs from new to create fixes all but one failure. I don't have time to look at it more right now.

hackartisan commented 7 years ago

Actually that one failure is one I had before since I don't have the dependency required for the docx thumnbnail generation. I'm going to push now and see what travis says about changing these from new to create.

hackartisan commented 7 years ago

this is passing now. quick somebody review it before dependencies break it again! 😆 @samvera/hyrax-code-reviewers

mjgiarlo commented 7 years ago

:clap: