Stop using deprecated URI.parser

piotrmurach / loaf

Manages and displays breadcrumb trails in Rails app - lean & mean.

MIT License

407 stars 24 forks source link

Stop using deprecated URI.parser #46

Closed dsazup closed 3 years ago

dsazup commented 3 years ago

Describe the change

Use recommended way to get the parser URI::DEFAULT_PARSER instead of deprecated URI.parser

Why are we doing this?

It has been deprecated in https://github.com/rails/rails/pull/39733

Benefits

Stops throwing a warning on 6.1 alpha.

Drawbacks

None

Requirements

Put an X between brackets on each line if you have done the item: [x] Tests written & passing locally? [x] Code style checked? [] Rebased with master branch? [] Documentaion updated?

dsazup commented 3 years ago

Hi Dainius,

Thanks for using loaf and submitting this PR!

Please see my comment re using unescape directly. Could you please verify that this change works and if it does resubmit PR? I will review and merge.

Hi Piotr,

I have actually made a mistake in the previous commit. It was supposed to be URI::DEFAULT_PARSER and not URI.DEFAULT_PARSER, sorry about that. In regards to your message about not using this default parser, I believe that the internal implementation you have linked to was also deprecated in ruby 2.7 https://github.com/ruby/ruby/blob/ruby_2_7/lib/uri/common.rb#L133 and removed in ruby 3 here https://github.com/ruby/ruby/commit/abbd3241522495e8d8634c0c01a42453f76ce6b8 :)

And thank you for creating this library, it's awesome!

piotrmurach commented 3 years ago

I'm persuaded. Thanks for providing references, learnt something new about Ruby.

Would you have time to add a note in the changelog file as well?

dsazup commented 3 years ago

Changelog updated 👍

piotrmurach commented 3 years ago

@dsazup Taking this opportunity, I'd like to get your thoughts on this issue https://github.com/piotrmurach/loaf/pull/42. I'm undecided (my thoughts are in the thread) and would like to resolve it one way or the other before I cut a new release.

coveralls commented 3 years ago

Coverage remained the same at 96.939% when pulling ffc5242d2f59cb1cdae6b498c6d18fbd966df166 on dsazup:uri-parser into 8f6a74c6ae3fd3ad5500dfbce28cc9c4fc28e35a on piotrmurach:master.

piotrmurach commented 3 years ago

Thank you! ❤️

piotrmurach commented 3 years ago

Released v0.10.0 with your contribution. Another notable change is this version is lighter and only relies on railties.