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

Translate reset types available for Server #56

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

glorpen
Copy link

@glorpen glorpen commented Dec 15, 2022

Currently allowed reset types for Server are returned to user in raw form (as in Hetzner api), but when programmatically requesting reset, same types are required to be translated.
Translate names in both places to be consistent.

User code that relies on raw reset names will break.

@aszlig
Copy link
Owner

aszlig commented May 8, 2023

Possibly related: #61

Copy link
Owner

@aszlig aszlig left a comment

Choose a reason for hiding this comment

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

Thanks, but as mentioned, I'm not sure whether it's a good idea to expose these aliases throughout the Python API. What would be the benefit instead of using the actual upstream API values?

@@ -42,7 +49,8 @@ def reset_types(self):
"""
if self._reset_types is None:
self._update_status()
return self._reset_types

return tuple(key for key, value in self._modes.items() if value in self._reset_types)
Copy link
Owner

Choose a reason for hiding this comment

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

This is an API change and rather than silently change its type and values, we should instead either opt for a deprecation process or wait for the next major release.

Comment on lines +8 to +13
_modes = {
'manual': 'man',
'hard': 'hw',
'soft': 'sw',
'power': 'power',
}
Copy link
Owner

Choose a reason for hiding this comment

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

I consider this to be an error that I introduced and rather than exposing this translation table further (and thus making it harder to get rid of), I'd instead remove it in the next major version and just use the values documented in the upstream API.

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