Skip to content

Conversation

kengallego
Copy link
Contributor

No description provided.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like having no-named parameters on method definition here (e.g. def put(path, body = {}, params = {}). This makes us very dependent on always putting things in the right order.

While @kengallego was developing the remove_tags_from_products, we had to modify the def delete to be able to add the body to it. To avoid conflicts in the calls that already exist, I told him to put the body = {} as the last parameter, which results in calls with the params first and the body after and vice versa.

I would rather use named params here (e.g. def delete(path, body: {}, params: {})) and use it like delete('products/batch/tags', body:) (on the remove_tags_from_products example).

Also we should think on using if body.any? and/or if params.any? for not sending always empty {} when it's not needed, for being consistent with the Beyond API.

What do you think @k4th, @kengallego, @citin? (Not saying we should do it on this task)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The no-named parameters it's an standard way of doing it. But we can decide to include params, body and headers in options or not. If its fit better how we use it, it's ok for me.

# Faraday
def get(url, params = nil, headers = nil) # &block
def post(url, body = nil, headers = nil) # &block

# Httparty
def get(path, options = {}, &block)
def post(path, options = {}, &block)

# RestClient
def self.get(url, headers={}, &block)
def self.post(url, payload, headers={}, &block)

Regarding the body in the DELETE method ... it's like sending a body in a GET method, it's not the standard. Should be ?ids=1,2,3 or ?ids[]=1,2,3 ... but .. it is what it is... have you tried this way? (I know that is not what the API docs says).

@kengallego kengallego force-pushed the add-products-endpoints branch from 656ff47 to 1a1fed7 Compare November 5, 2024 10:14
@kengallego kengallego merged commit bafd0d6 into ePages-de:wip-v2 Nov 5, 2024
@kengallego kengallego deleted the add-products-endpoints branch November 5, 2024 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants