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

Add smart_handler as the default for api.Client #1196

Merged
merged 1 commit into from
Apr 18, 2019

Conversation

rochacbruno
Copy link
Member

This handler can decide which handler to use based
on content response.

@nixocio
Copy link

nixocio commented Apr 10, 2019

@rochacbruno I do like this idea. To have a generic response_handler for Pulp 3 that the user does not need to know in advance what kind of status code, or a pagination will be part of the response. We need to test this to assure that we are not missing any edge case. What are about we call it generic_handler or general_handler?

@rochacbruno
Copy link
Member Author

@kersommoura

I suggest something like

  • base_handler
  • default_handler
  • dispatch_handler (ugly name, but very meaningful)

@nixocio
Copy link

nixocio commented Apr 11, 2019

@rochacbruno naming anything it is so hard. @bherrin3 can decide.

@nixocio
Copy link

nixocio commented Apr 11, 2019

In my mind the name has to show the intent...and the intent it is to be a general reponse_handler for Pulp 3. One that the user will pick in the cases that user does not know what response will get back from Pulp 3. But I am fine with any choice.

@rochacbruno
Copy link
Member Author

@kersommoura if we want to show intent, then smart_handler is ok!

Like other tech examples:

  • Smart Phone
  • Smart Templates

We want to express the idea that

This handler can make its own choice based on context

I vote for smart_ or dispatch_

@rochacbruno
Copy link
Member Author

Or auto_handler - automatic_handler

@bherrin3
Copy link
Contributor

hmm.. I think you may already have the right name with smart_handler. That has intelligent to decide which is the appropriate handler given the case. I don't think it infers anything other than it uses logic to decide the appropriate handler.

Else, a method name that names the action is doing. I am not sure why this is post-pended with _handler. However, of the names presented, smart_handler is the best I have seen.

Using base_ or default_ assumes there is a base class (subset) that is common to all handlers you are using, which is not the case here. The appropriate handler is being chosen. Maybe the function name shouldn't have handler in it since it is chosing and returning a handler and not actually a handler?

I don't have a dog in this race. I would just choose something and comment what it does. :)

@rochacbruno
Copy link
Member Author

Here's a good name:
client = api.Client(reponse_handler=api.use_this_if_you_dont_know_how_to_choose_a_handler).get(...)

😄

Why so serious?

lets call it ninja_handler, super_handler, handler_with_lasers or use some buzzword like awesome_handler

@rochacbruno
Copy link
Member Author

Lets stick with smart_handler ? So we can put some test cases and merge

@nixocio
Copy link

nixocio commented Apr 17, 2019

@rochacbruno, I am reviewing this one. Good job. Nice unittests.

I updated the pagination test to use the smart_handler, and test failed. I am evaluating this one right now.

@nixocio
Copy link

nixocio commented Apr 17, 2019

@rochacbruno, it was an error on my side. I tested a few tests for Pulp 3 using the smart_handler, and all passed.

Copy link

@nixocio nixocio left a comment

Choose a reason for hiding this comment

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

Just suggestions. Not blockers.

pulp_smash/api.py Outdated Show resolved Hide resolved
pulp_smash/api.py Show resolved Hide resolved
pulp_smash/api.py Outdated Show resolved Hide resolved
pulp_smash/api.py Outdated Show resolved Hide resolved
This handler can decide which handler to use based
on content response.
@nixocio nixocio merged commit c9ca482 into pulp:master Apr 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants