Deprecation cleanup #1466
Deprecation cleanup #1466
Conversation
| def additional_response_formats format | ||
| blacklight_config.index.respond_to.each do |key, config| | ||
| format.send key do | ||
| case config | ||
| when false | ||
| raise ActionController::RoutingError, 'Not Found' | ||
| when Hash | ||
| render config | ||
| if config[:nothing] # nothing param deprecated in rails, but still in configs | ||
| head :ok, config.reject { |k, v| k == :nothing } |
There was a problem hiding this comment.
Unused block argument - v. If it's necessary, use _ or _v as an argument name to indicate that it won't be used.
There was a problem hiding this comment.
config.except(:nothing) ought to work here, yeah?
| def additional_response_formats format | ||
| blacklight_config.index.respond_to.each do |key, config| | ||
| format.send key do | ||
| case config | ||
| when false | ||
| raise ActionController::RoutingError, 'Not Found' | ||
| when Hash | ||
| render config | ||
| if config[:nothing] # nothing param deprecated in rails, but still in configs |
There was a problem hiding this comment.
Do we need to handle this case? Wouldn't it be better to get downstream applications to update their config? At the very least, we should issue our own deprecation warning so they do.
There was a problem hiding this comment.
Data migration of existing production configs is outside my intended scope for this PR, but I otherwise agree with you. I needed to catch it to resolve the last outstanding deprecation warnings. Not sure what to advise the migrate to. Since previous configs were passed to render, that was easy, but the deprecation we are trying to avoid is precisely that we can't do render nothing: true anymore. Seems like we will need a conditional regardless.
There was a problem hiding this comment.
My point is, this isn't blacklight's deprecation to clean up. We're just an intermediate party, and downstream applications will need to get updated. I'm +0, though, on capturing their deprecation warning and giving them an equivalent but more accurate deprecation warning.
There was a problem hiding this comment.
Blacklight set the config syntax and behavior. To say it is the downstream app's problem means breaking those apps on rails 5.1 for the same configs blacklight currently uses (and maybe even generates).
There was a problem hiding this comment.
The parameters to respond_to are intended and expected to mirror the Rails parameters, we're not trying to invent our own render API. If downstream applications ignore the deprecation warning and upgrade to Rails 5.1 without heeding those warnings, that's on them.
There was a problem hiding this comment.
How do you expect them to handle the case currently borne by render :nothing? If we don't expose head, there is no option they can pass via this switch to render that 5.1 will support. That's not their failure to resolve deprecations, if we're the ones forcing them to pass options to render in the first place.
There was a problem hiding this comment.
This seems acceptable:
config.index.respond_to.txt = Proc.new { head :ok }
Also acceptable might be config.index.respond_to.txt = { body: "" }, or similar variations.
There was a problem hiding this comment.
Ok, I see what you mean. You want me to back this change out?
1 similar comment
- Use `let` statements, remove class instance vars - Use common before blocks - Correct indent formatting - Remove redundant mocks! - Removes rails deprecations around render calls - Remove rails 3 cruft
| it "does not call the supplied method" do | ||
| expect(controller).to receive(:validate_like_params).and_return(false) | ||
| expect(controller).not_to receive(:perform_like) | ||
| expect { post :like }.not_to raise_error |
There was a problem hiding this comment.
This is now the sole failing test on Rails 4.
Note: the old version of the test was bogus. The post was already made at 683 before validate_like_params was re-stubbed false.
There was a problem hiding this comment.
Worse than that -- I'm not sure the post was even triggered. It seems like we're not effectively handling invalid input (in actual use, it'll fail over to the original template.. but still)
There was a problem hiding this comment.
For now, I'd suggest marking it as pending, filing a ticket, and moving on.
1 similar comment
| # Collects formats that this doc can export as. | ||
| # Returns a hash, keys are format short-names that can | ||
| # be exported. Hash includes: | ||
| # :content-type => mime-content-type | ||
| # :content-type => { content_type: content_type } |
There was a problem hiding this comment.
I think this was intended to be a placeholder for a string, so something like this would be more correct:
:content_type => 'sample/content-type'There was a problem hiding this comment.
Yeah, it seems like a bad comment. The method returns a hash, with values that are themselves hashes. The example here was presumably demonstrating the contents of a value, not the entire hash. Maybe we can rework the comment to be better?
There was a problem hiding this comment.
Yeah, it was clearly half-baked previously. I was just trying to capture that there was internal structure (values are hashes). I'll touch it up and merge.
1 similar comment
This updated version is from Curation Concerns.
It never actually posted to the controller. With a post it fails on Rails 4 but passes on Rails 5. Needs clarification.
Fixes #1464
Fixes #1465
Fixes #1463
Cleans up catalog_controller_spec considerably.