-
Notifications
You must be signed in to change notification settings - Fork 2
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
bundle_context class to bundle_context AR Model #232
Conversation
Pull Request Test Coverage Report for Build 742
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a bunch of comments, but they're all nitpicks. i'd like to see them all addressed (even if just to say they're all non-issues), but i'd also be happy to see this merged and to see any changes made in follow-on issues/PRs.
When method by method and moved over methods in BundleContextTemp to BundleContext AR model. Included validations as well has methods that are used throughou the application. Wrote RSpect tests for the methods moved over, and left ticket numbers on outstanding work.
aef146c
to
b714d79
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
buncha comments perhaps worthy of follow-on tickets and one possible discussion about config/project
stuff (better F2F than on here)
File.join(bundle_dir, rel_path) | ||
end | ||
|
||
# TODO: BundleContext is not really a logical home for this util method |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hrmm - not sure I agree, as reading the file from the user supplied location is needed to flesh out the bundle context data ...
Also - why is this a class method? should it be a private instance method directly operating on path_in_bundle(manifest) ? I think this is the only place where we read a csv file, unless @atz envisions writing out discovery report data as a csv (which could be a good idea, if folks work with the old discovery report that way - as it was designed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, i'd left this comment a while back (after splitting out BundleContext
from Bundle
, but before sarav moved it to its own file). at that time, Bundle
also used import_csv
. but i just looked, and it doesn't use it any longer. so yeah, now it could just be a private instance method in this class, which is now a logical home for it.
i think that also means we can get rid of this delegation, and the require 'csv'
at the top of bundle.rb
:
https://github.com/sul-dlss/pre-assembly/blob/master/app/lib/pre_assembly/bundle.rb#L3
https://github.com/sul-dlss/pre-assembly/blob/master/app/lib/pre_assembly/bundle.rb#L41-L43
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cleaned up in #244
When method by method and moved over methods in BundleContextTemp to
BundleContext AR model. Included validations as well has methods that
are used throughou the application.
Wrote RSpect tests for the methods moved over, and left ticket numbers
on outstanding work.
closes #176
closes #182