move duplication in services to Connection.get and Connection.post

[bedrock-adam]

Motivation (Why)

Tony and I will soon need to raise AccessTokenRejected and limiting the duplication to the Connection methods helps a lot with that.

Later we can reduce the duplication in Connection too - just not yet.

Consequence

No change in terms of callers as Connection is encapsulated.

[abreckner]

I am wondering how much similarity there will be in the PUT, POST and DELETE actions and if we could maybe abstract this out into a generic Response object, e.g.
response = Response.new(Faraday.new(url(endpoint: endpoint), headers: headers).get, element_name)
but we will see when we get there.

Might be too early to abstract/generalise.

:)

[bedrock-adam]

Yeah I think so @abreckner :).

Like we say it's okay to duplicate some of this when we work on those endpoints. We'll be in a good position to refactor out the noise in either this or another PR when we've got a few examples going.

[bedrock-adam]

As I mentioned in the description we can clean this up later at least it's in once place now.

[bedrock-adam]

Think we made a good call resisting the temptation to DRY this early on too @abreckner 😄.

[abreckner]

Overall this is great (moving the error handling into the connection which is shared). Much needed! 👍

Just a small thing, I am just wondering if it's a little more readable to still have a response variable

e.g.

response = Connection
        .new(access_token: access_token, xero_tenant_id: xero_tenant_id)
        .post(endpoint: "client.api/add", data: client.to_xml(root: "Client"))
response["Client"]

I am just wary that if we have too much method chaining the code can get a little hard to follow. We may even want a connection variable too

connection = Connection.new(access_token: access_token, xero_tenant_id: xero_tenant_id)
response = connection.post(endpoint: "client.api/add", data: client.to_xml(root: "Client"))
response["Client"]

But let me check the Ruby guidelines. This might just be a personal thing for me. ;)

[bedrock-adam]

Like your first example Tony. Seems like a good compromise between too much chaining and too much assignment. I'll update.

[bedrock-adam]

We can remove the chaining on Connection later by making it a module instead of an object which I think will also simplify things too.

[abreckner]

So I was doing some reading and it appears that method chaining is ok/good practice when you are working on the same object, but as these are returning different objects it can make debugging quite confusing.

e.g. if you were working with Arel where a query is always returned then Person.where().order().etc() where it is always working on the query object then it's good practice.

If each method returns a different object it can get confusing.

Elixir has a pipe operator (|>), but Elixir is functional which means you are only doing data transformations with each pipe which is slightly different.

We can have a chat about it in stand up though.
:)

[bedrock-adam]

Yeap good points I got carried away - have updated :P

[abreckner]

Overall this is great, but I would like to have a quick chat about method chaining (non blocking).
:)

[abreckner]

Ah I see you updated this. Great stuff!

[abreckner]

Reference article: https://stackoverflow.com/questions/1103985/method-chaining-why-is-it-a-good-practice-or-not