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

Refactor parameter sorting #1162

Merged
merged 2 commits into from Jul 7, 2020
Merged

Refactor parameter sorting #1162

merged 2 commits into from Jul 7, 2020

Conversation

@wishdev
Copy link
Contributor

@wishdev wishdev commented Jun 30, 2020

Description

This is a quasi-second attempt at #469 which respects the comment in it #469 (comment)

As opposed to adding any new options - this simply moves the sort call into a method which can be overridden as opposed to requiring the "cut and paste" of the standard modules into a custom module.

So with this patch one can accomplish no param sorting by simply adding

module Faraday
  module NestedParamsEncoder
    def self.sort_params(_); end
  end

  module FlatParamsEncoder
    def self.sort_params(_); end
  end
end

To their code, because sort_params can be overridden.

Again, I completely respect the desire to keep it clean - I'm just wondering if this slight refactor would be ok.

@iMacTia
Copy link
Member

@iMacTia iMacTia commented Jun 30, 2020

Hey @wishdev, appreciate you trying to fit this in following the first feedback in the other PR.
I think I can relate to Mislav's comment, it's really easy to add small tweaks into your gem, until over time the number of tweaks grows beyond control.

The biggest issue I have with your change is that you're changing the default behaviour of the gem.
This might fit your requirements, but may potentially break other existing implementations.
For that reason, we would need to change your PR so that sort_params actually does not do any sorting, and you'd have the opportunity to override it in your project, in order to make it sort them for real.
As you can imagine this would be quite dumb (a method called sort_params that doesn't sort 😄).
Moreover, you'd still need to override this new method in your project.

Luckily, Ruby is an OOP language and as such it should already give you all the tools you need, so let me show you what I think it's the best solution in your case and, most probably, what Mislav was also referring to in his comment:

class MyEncoder < Faraday::NestedParamsEncoder # or FlatParamsEncoder
  self.encode(params)
    super
    params.sort!
  end
end

# Then later when you initialise your Faraday Connection
conn = Faraday.new(url, request: { params_encoder: MyEncoder })

There you go, no change necessary in the Faraday gem itself, and just 6 lines of code in your project.
I haven't tested the code above, but I'm sure that will give you the idea of what I mean.

I'm closing this PR in the hope that the above helps you with what you're trying to do, but if you have any questions then feel free to follow-up.

@iMacTia iMacTia closed this Jun 30, 2020
@wishdev
Copy link
Contributor Author

@wishdev wishdev commented Jun 30, 2020

I apologize but you misread the code. The current implementation sorts. I do not want a sort and therefore seek the change to allow the sort_params addition to be a passthrough.

There is no change at all to the default and current implemenation. This is simple a decomposition of the encode method to allow for a more granular override capability.

@iMacTia
Copy link
Member

@iMacTia iMacTia commented Jun 30, 2020

I'm sorry @wishdev, I most definitely did!

To my surprise, we currently sort parameters indeed. I was unaware of that bit but it seems like it has been like that over the past 8 years 😮.

Based on everything I said above, your approach makes sense to me and it will allow devs to override that opinionated behaviour when necessary

@iMacTia iMacTia reopened this Jun 30, 2020
@iMacTia
Copy link
Member

@iMacTia iMacTia commented Jun 30, 2020

I was curious to see if tests were failing, but there seems to be an issue with one of the dependencies, I'll try to have a look when I get a chance

Copy link
Member

@iMacTia iMacTia left a comment

@wishdev Tests are green now, it seems like it was a temporary issue with GitHub Actions.
I'm OK with this PR now, but I wanted to propose a "compromise" solution as well before we proceed.
I've reviewed #469 and I actually don't think this solution is much better than adding a new option, BUT sticking that new option into the Faraday connection caused changes in multiple files, which I don't personally like.

I'd be happy to review and approve an alternative solution where instead of introducing a new method, we introduce a class-level option in both the encoders to enable or disable the sorting of parameters (and of course it would default to being enabled for backwards-compatibility).
It would work like this:

# The same can be done for Faraday::NestedParamsEncoder
Faraday::FlatParamsEncoder.sort_params = false # default value is true

The change would be limited to the encoders (thus not touching the connection) and changing the behaviour would be as easy as to set the new option to false, with no need to override methods.
What do you think of this revised, middle-way proposal @wishdev?

@wishdev wishdev force-pushed the wishdev:param_sorting branch from 643b13b to 17dc991 Jul 1, 2020
@wishdev
Copy link
Contributor Author

@wishdev wishdev commented Jul 1, 2020

# The same can be done for Faraday::NestedParamsEncoder
Faraday::FlatParamsEncoder.sort_params = false # default value is true

The change would be limited to the encoders (thus not touching the connection) and changing the behaviour would be as easy as to set the new option to false, with no need to override methods.
What do you think of this revised, middle-way proposal @wishdev?

I believe it is an excellent idea @iMacTia. I started with the add nothing concept and I like middle grounds. I've pushed the change as you requested along with specs.

@iMacTia
iMacTia approved these changes Jul 3, 2020
Copy link
Member

@iMacTia iMacTia left a comment

Thanks @wishdev, I really appreciate you trying to satisfy Mislav's initial comment.
I'm happy with this new implementation, thanks for your patience and contribution!
I'm checking why Rubocop is complaining, most probably not your PR's fault so I'll investigate it separately 👍

@iMacTia
Copy link
Member

@iMacTia iMacTia commented Jul 6, 2020

@wishdev I did some digging about the Rubocop issue and I found this comment.
The reason seems to be that your lines FlatParamsEncoder.sort_params = true and NestedParamsEncoder.sort_params = true are inside the module Faraday block, causing Rubocop not to consider it a namespace (as it should be).

May I ask you one last change to your PR by moving those lines outside the module Faraday block?
That should fix the Rubocop issues 👍

Copy link
Member

@iMacTia iMacTia left a comment

See comment above

@wishdev
Copy link
Contributor Author

@wishdev wishdev commented Jul 6, 2020

My apologies for not understanding what the rubocop issue was concerning.

I believe this should move things along.

@iMacTia
iMacTia approved these changes Jul 7, 2020
Copy link
Member

@iMacTia iMacTia left a comment

No worries! It took me some time as well to dig it out, thanks for fixing it so quickly 🙌
Code looks great, tests are all green and rubocop is happy 🎉
LGTM

@iMacTia iMacTia merged commit e02a8c1 into lostisland:master Jul 7, 2020
5 of 6 checks passed
5 of 6 checks passed
linting
Details
build (2.4)
Details
build (2.5)
Details
build (2.6)
Details
build (2.7)
Details
codeclimate 1 issue to fix
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.