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

Merging connect kwargs and init kwargs with priority #31

Merged
merged 7 commits into from
Jun 5, 2023

Conversation

sallory
Copy link
Contributor

@sallory sallory commented Jun 1, 2023

#14

Please feel free to correct me and ask me to fix/add something.

@Lancetnik
Copy link
Owner

Hi there! It's a good starting point, but I am thinking about merging args and kwargs both

Broker("smth-url")
Broker.connect(url="smth-url")

Should be a valid case, but in your code it leads to exception.
Take a look here, it can be helpful to merge all args and kwargs to one dict

@sallory
Copy link
Contributor Author

sallory commented Jun 1, 2023

Thanks for your comment.
I got you, I will take a look at your code and update this pr soon

@Lancetnik
Copy link
Owner

Do you need some help with this?

@sallory
Copy link
Contributor Author

sallory commented Jun 4, 2023

Hi, didn't have time for this task.
But I'm thinking about how to detect the arguments list for the particular broker.
Like I want to use your args_to_kwargs, but we don't know the list of arguments.

The first approach would be, storing the arguments list in the broker class:

class RabbitBroker:
    __connect_arguments: list = ['url']
    
    
class KafkaBroker:
   __connect_arguments: list = ['bootstrap_servers']

and we can do like this:

class BrokerUsecase:
        async def connect(self, *args: Any, **kwargs: Any) -> Any:
            if self._connection is None:
                _kwargs = args_to_kwargs(self.__connect_arguments, args, kwargs)
                self._connection = await self._connect(**_kwargs)
            return self._connection    

What do you think?

@Lancetnik
Copy link
Owner

I suppose, you should move all broker init arguments from .pyi to real .py files and get information about them by inspect module.

@sallory
Copy link
Contributor Author

sallory commented Jun 5, 2023

Hi @Lancetnik
Updated the pr

  • changed the signature of _connect method to accept only kwargs
  • the list of key-word only params changed in some of .pyi files and Brokers' __init__ methods, because for example nats.connect wants only 1 pos argument (servers)

@Lancetnik
Copy link
Owner

Wow! That was a greate work. I will just take a look and merge it soon.

@Lancetnik Lancetnik merged commit f1eaa48 into Lancetnik:main Jun 5, 2023
9 checks passed
@Lancetnik
Copy link
Owner

@sallory thanks for your help! Can I wait any more PRs from u?

@sallory
Copy link
Contributor Author

sallory commented Jun 5, 2023

@Lancetnik For sure. If I will have time, I would like to help you meet your goals with this project

@sallory sallory deleted the connect_merging_kwargs branch June 5, 2023 16:34
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

2 participants