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

V3 resource and image_resource tests improved #29

Merged
merged 5 commits into from
Jul 25, 2017

Conversation

ndushay
Copy link

@ndushay ndushay commented Jul 21, 2017

Nothing earth shaking here:

  • fixed a typo in Service
  • added profile to Service, a commonly used key
  • Resource - beefed up tests, validation, specificity
  • ImageResource - ditto

If it looks good to you, merge it.

@ndushay ndushay force-pushed the v3-image_resource-tests branch from e21f37e to 52c15cc Compare July 22, 2017 01:11
Copy link
Member

@jmartin-sul jmartin-sul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one question, one very minor style nitpick repeated a few times. if putting multi-line blocks in curly braces is a convention that's already in use in the project, i'd disregard those comments entirely. not a thing i care one way or the other about in the first place, and i wouldn't detract from consistency just to mollify a style checking complaint i've seen on other projects.

but overall, definite 👍, happy to merge as-is if you think that's the right thing to do. no substantial changes to suggest from me.

let(:file_id) { 'https://example.org/file/abc666/ocr.txt' }
let(:file_type) { 'Document' }
let(:file_mimetype) { 'text/plain' }
let(:resource_object) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think i've seen rubocop complain about not using the do form for multiline blocks. not a thing that bothers me personally, and not sure how much others care about that convention, but just FYI in case that makes you want to change this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yeah - I've seen that too. At this point, I think I'm inclined to just let it ride.

}]
)
}
let(:resource_object_w_login) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto multiline block on this and let(:login_service) (if you change the one in the preceding test)

def validate
super

unless self['id'] =~ /^https?:/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the superclass takes care of validating against the more general URI::regexp, right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does now! Good catch!

end
describe 'realistic examples' do
let(:img_id) { 'https://example.org/image/iiif/abc666' }
let(:image_v2_service) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto multi-line block if you decide to change

describe 'stanford' do
describe 'thumbnail per purl code' do
let(:thumb_id) { "#{img_id}/full/!400,400/0/default.jpg" }
let(:thumb_object) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto multi-line block if you decide to change

end
describe 'full size per purl code' do
let(:full_id) { "#{img_id}/full/full/0/default.jpg" }
let(:image_object) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto multi-line block if you decide to change

describe 'requires login' do
let(:service_label) { 'login message' }
let(:token_service_id) { 'https://example.org/iiif/token' }
let(:login_service) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto multi-line block if you decide to change

}]
)
}
let(:image_object_w_login) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto multi-line block if you decide to change

end
end
describe 'image examples from http://prezi3.iiif.io/api/presentation/3.0' do
let(:image_object) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto multi-line block if you decide to change

# "tiles": [{"width": 512, "scaleFactors": [1,2,4,8,16]}]
# }
# }
let(:img_obj) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto multi-line block if you decide to change

@ndushay ndushay force-pushed the v3-image_resource-tests branch from 7de92e1 to 7d6f96a Compare July 25, 2017 05:51
@ndushay
Copy link
Author

ndushay commented Jul 25, 2017

@jmartin-sul I've added the validate_uri call to the abstract_resource#validate method as your comment implied. Yay!

@jmartin-sul jmartin-sul merged commit 7164dc7 into development Jul 25, 2017
@jmartin-sul jmartin-sul deleted the v3-image_resource-tests branch July 25, 2017 08:55
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

Successfully merging this pull request may close these issues.

2 participants