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

Update dependencies #844

Merged
merged 3 commits into from Oct 27, 2016
Merged

Update dependencies #844

merged 3 commits into from Oct 27, 2016

Conversation

cbeer
Copy link
Member

@cbeer cbeer commented Oct 17, 2016

Argo also needed changes to fixture object loading; whereas in AF 6.0, the ActiveFedora::Base class provided access to all datastreams, in later versions these needed to be made explicit. The easy thing to do was to simply cast objects to their expected classes.

@cbeer cbeer force-pushed the update-dependencies branch 3 times, most recently from 04f3afd to 5743c6d Compare October 19, 2016 14:37
@coveralls
Copy link

coveralls commented Oct 19, 2016

Coverage Status

Coverage decreased (-1.9%) to 82.684% when pulling 5743c6d on update-dependencies into 2438900 on master.

end
end
result.instance_eval do
def save ; true ; end

Choose a reason for hiding this comment

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

Space found before semicolon.

# end
# end
ds.instance_eval do
def save ; true ; end

Choose a reason for hiding this comment

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

Space found before semicolon.

end
end

# stub item and datastream repo access methods
result.datastreams.each_pair do |dsid, ds|

Choose a reason for hiding this comment

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

Unused block argument - dsid. If it's necessary, use _ or _dsid as an argument name to indicate that it won't be used.

@coveralls
Copy link

coveralls commented Oct 19, 2016

Coverage Status

Coverage remained the same at 84.58% when pulling f54657f on update-dependencies into 2438900 on master.

@cbeer cbeer changed the title Update dependencies [WIP] Update dependencies Oct 19, 2016
@cbeer cbeer changed the title [WIP] Update dependencies Update dependencies Oct 24, 2016
@coveralls
Copy link

coveralls commented Oct 24, 2016

Coverage Status

Coverage remained the same at 84.58% when pulling f406723 on update-dependencies into 2438900 on master.

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.

a couple of small questions/requests, but overall, LGTM

@@ -8,7 +8,7 @@
@item = instantiate_fixture(@druid, Dor::Item)
@user = User.find_or_create_by_webauth double('WebAuth', :login => 'sunetid', :logged_in? => true, :attributes => {'DISPLAYNAME' => 'Example User'}, :privgroup => '')
allow(Dor).to receive(:find).with("druid:#{@druid}").and_return(@item)
allow(Dor::Item).to receive(:find).with("druid:#{@druid}").and_return(@item)
allow(Dor).to receive(:find).with("druid:#{@druid}").and_return(@item)
Copy link
Member

Choose a reason for hiding this comment

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

since it's an allow, and the two lines are now identical, could we just remove this?

@@ -39,6 +39,7 @@ end

if ['test', 'development'].include? Rails.env
require 'jettywrapper'
Jettywrapper.hydra_jetty_version = 'v7.3.0'
Copy link
Member

Choose a reason for hiding this comment

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

could you add a comment explaining the version specification?

@coveralls
Copy link

coveralls commented Oct 27, 2016

Coverage Status

Coverage remained the same at 84.58% when pulling ff856c8 on update-dependencies into 2438900 on master.

@jmartin-sul jmartin-sul merged commit 12287a3 into master Oct 27, 2016
@jmartin-sul jmartin-sul deleted the update-dependencies branch October 27, 2016 01:31
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.

None yet

4 participants