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

'media' content-type, not 'smpl' #521

Merged
merged 5 commits into from
Aug 16, 2019
Merged

'media' content-type, not 'smpl' #521

merged 5 commits into from
Aug 16, 2019

Conversation

ndushay
Copy link
Contributor

@ndushay ndushay commented Aug 15, 2019

do not merge until vetted by stakeholder!!!

Note: this needs to be has been vetted by @andrewjbtw on preassembly stage before it is so it can be merged to master. Hence it is a draft until then.

Changes to address issue #515 (change refs to "smpl" content-type to "media").

Replaces draft PR #518 -- d5d6daa shows why travis build was failing -- our laptop mac os-x didn't care about the case of the initial "M" but travis did -- the directory name needs to match the project name.

In the course of looking for the solution, I found some other references to "smpl" which I changed, and I think a couple corrections from 60d562f. If you're curious, review each commit in turn.

@coveralls
Copy link

coveralls commented Aug 15, 2019

Coverage Status

Coverage remained the same at 88.752% when pulling 8db061b on media-not-smpl into 58245ab on master.

@ndushay
Copy link
Contributor Author

ndushay commented Aug 15, 2019

@andrewjbtw this is deployed to sul-preassembly-stage; can you please vet the changes and let us know if this addresses your concerns for issue #515 ?

@ndushay ndushay changed the title 'media' content-type, not 'smpl' [WIP] 'media' content-type, not 'smpl' Aug 15, 2019
@ndushay
Copy link
Contributor Author

ndushay commented Aug 15, 2019

when merged, please be sure to delete this branch and smpl-to-media-refs as well, if it's not already gone.

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.

overall, looks good to me. i noticed one remaining instance of the string smpl that seemed like it needed removing (in the rubocop todo file). the rest of the instances i found by git grep were in release notes for old versions, links to consul docs, and test fixtures/instances; those things felt either like they shouldn't be changed, or weren't worth changing.

for the rubocop thing, i put up a PR to this PR (#522). wouldn't hold this PR up for that one.

@andrewjbtw
Copy link

This looks good. We tested on preassembly-stage with an example from a real accession and everything checked out.

@ndushay ndushay changed the title [WIP] 'media' content-type, not 'smpl' 'media' content-type, not 'smpl' Aug 16, 2019
@ndushay
Copy link
Contributor Author

ndushay commented Aug 16, 2019

when merged, please be sure to delete this branch and smpl-to-media-refs as well, if it's not already gone.

@jmartin-sul jmartin-sul merged commit 5579b03 into master Aug 16, 2019
@jmartin-sul jmartin-sul deleted the media-not-smpl branch August 16, 2019 16:27
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.

6 participants