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

Convert to using a class. #1

Merged
merged 3 commits into from Aug 4, 2021
Merged

Convert to using a class. #1

merged 3 commits into from Aug 4, 2021

Conversation

benjmarshall
Copy link
Contributor

I'd like to propose moving this library to using a class. A "gotify" object could then hold the valid configuration for interacting with a specific server/application id/client id. This will make the library more useful for applications which want to interact with multiple servers or multiple application targets on a single server.

I have a specific use case for this library trying to integrate Gotify as a notification platform for Home Assistant. A full implementation of a configurable notification platform would benefit massively from this change.

This will be a breaking change for any applications which already rely on this library so should be discussed before any merge.

@d-k-bo what are your thoughts on this?

@d-k-bo
Copy link
Owner

d-k-bo commented Aug 3, 2021

Great suggestion!
I think compatibility issues can be avoided for the most part by exposing the methods of a gotify object on module level:

# --- Support legacy functions ------------------------------------------------

_gotify_obj = gotify()

config = _gotify_obj.config
get_applications = _gotify_obj.get_applications
create_application = _gotify_obj.create_application
update_application = _gotify_obj.update_application
delete_application = _gotify_obj.delete_application
upload_application_image = _gotify_obj.upload_application_image
get_messages = _gotify_obj.get_messages
create_message = _gotify_obj.create_message
delete_messages = _gotify_obj.delete_messages
delete_message = _gotify_obj.delete_message
get_clients = _gotify_obj.get_clients
create_client = _gotify_obj.create_client
update_client = _gotify_obj.update_client
delete_client = _gotify_obj.delete_client
get_current_user = _gotify_obj.get_current_user
set_password = _gotify_obj.set_password
get_users = _gotify_obj.get_users
create_user = _gotify_obj.create_user
get_user = _gotify_obj.get_user
update_user = _gotify_obj.update_user
delete_user = _gotify_obj.delete_user
get_health = _gotify_obj.get_health
get_plugins = _gotify_obj.get_plugins
get_plugin_config = _gotify_obj.get_plugin_config
update_plugin_config = _gotify_obj.update_plugin_config
disable_plugin = _gotify_obj.disable_plugin
get_plugin_display = _gotify_obj.get_plugin_display
enable_plugin = _gotify_obj.enable_plugin
get_version = _gotify_obj.get_version

BTW, why did you move the project from using a gotify.py file to a gotify/__init__.py structure? Does this have any implications for the import process, are there any conventions regarding this or is it just personal preference? I thought it would be cleaner to use a flat hierarchy.

@benjmarshall
Copy link
Contributor Author

Good call with the compatibility functions! I've added those and tested locally and all seems good.

The hierarchy change is just habit. I believe it shouldn't make a difference for packaging. Your right though it does look cleaner for a single file to just use a flat structure. I've reverted that change.

I've bumped the version to 0.2.0 and also tagged this as compatible with python 3.8 and up rather than just 3.9 and up. This is mostly for use with Home Assistant as they are currently maintaining support for 3.8. Test locally with 3.8 and all seems to work just fine. Can't see any obvious that should depend on 3.9 either

@d-k-bo It would be great if you could review these updates (maybe test them yourself too, just to make sure) and merge them if your happy?

Copy link
Owner

@d-k-bo d-k-bo left a comment

Choose a reason for hiding this comment

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

Hey, thanks for your excellent work!
I will merge the changes and publish a new version to PyPi.

@d-k-bo d-k-bo merged commit 36d8a5a into d-k-bo:master Aug 4, 2021
@d-k-bo d-k-bo mentioned this pull request Nov 27, 2021
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