Skip to content

Make package plugable#52

Draft
thomasborgen wants to merge 3 commits into
mainfrom
rewrite/make-package-plugin-based
Draft

Make package plugable#52
thomasborgen wants to merge 3 commits into
mainfrom
rewrite/make-package-plugin-based

Conversation

@thomasborgen

@thomasborgen thomasborgen commented Dec 21, 2021

Copy link
Copy Markdown
Owner

Still very much a WIP but the idea is that we register functions with decorators, and get them when needed.

So in package storage_bucket.aws.create_bucket we have function aws_create_bucket() with is decorated with @register_create_bucket(platform=Platform.aws)
And then in root package storage_bucket.create_bucket we have the api function create_bucket() which takes a enum Platform, and the get_create_bucket(platform) function in storage_bucket.registries retrieves the registered function for the correct platform.

This could work quite nicely.

Another nice thing about this approach is that the code is nice and separated. This means that the aws_create_bucket function can call its own aws_get_client() without us having to for example resolve it earlier in the pipe and pass it to the function.

Right now I've tried with BaseCreateBucket input dataclass that the platform spesific functions can subclass, but maybe some other approach is better.

The main.py file is there for testing purposes only and can be run with poetry run python storage_bucket

@ChameleonTartu got any thoughts on this?

I can say that I tried having CloudClient and have that subclassed by each platform based Client, but the abstract methods would not work with any other input that the CloudClient had demanded. So I couldn't like have a BaseInput and SpecificAWSInput(BaseInput) as type parameters. :( Might have just been mypy issue but after a bit of reading it seems like it wasn't the best of ideas.

@ChameleonTartu

Copy link
Copy Markdown
Collaborator

@thomasborgen I quickly looked through the implementation. I cannot formulate now what I would change but the implementation looks similar to what we discussed. I need to sleep on it.

Note: Maybe it is because of typing but I got the first impression that it is slightly more complicated than necessary. Maybe it is only an impression.

@thomasborgen

thomasborgen commented Dec 29, 2021

Copy link
Copy Markdown
Owner Author

@thomasborgen I quickly looked through the implementation. I cannot formulate now what I would change but the implementation looks similar to what we discussed. I need to sleep on it.

Note: Maybe it is because of typing but I got the first impression that it is slightly more complicated than necessary. Maybe it is only an impression.

@ChameleonTartu

Yes, the typing with the overloading is quite annoying yes. But we should keep typing and some overhead in this package, is worth it when getting correct type warnings for users.

One way to make it a bit easier is to use singledispatch from stdlib or classes to let the type of the argument decide what function to call. so its like this

params = GCPCreateBucket(...)
create_bucket(params)

@thomasborgen

Copy link
Copy Markdown
Owner Author

We could also let people use env vars to get the correct platform so that we don't have to provide platform="" in the functions.

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.

2 participants