hcloud-python icon indicating copy to clipboard operation
hcloud-python copied to clipboard

fix!: make `datacenter` argument optional when creating a primary ip

Open jooola opened this issue 1 year ago • 4 comments

Create a primary ips assigned to a resource without having to pass the datacenter argument.

jooola avatar Feb 19 '24 17:02 jooola

This is a breaking change, but I'd argue it is a necessary bug fix.

@apricote Should we release a v2, and try to pack a few breaking changes in addition to this one?

jooola avatar Feb 19 '24 17:02 jooola

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 92.97%. Comparing base (6b977e2) to head (ee62dea).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #363   +/-   ##
=======================================
  Coverage   92.96%   92.97%           
=======================================
  Files          64       64           
  Lines        2873     2874    +1     
=======================================
+ Hits         2671     2672    +1     
  Misses        202      202           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Feb 19 '24 17:02 codecov[bot]

This is a breaking change, but I'd argue it is a necessary bug fix.

@apricote Should we release a v2, and try to pack a few breaking changes in addition to this one?

To be sure, this is breaking change for the reasons outlined in https://github.com/hetznercloud/hcloud-python/issues/223?

Is it possible to just define the = None for the datacenter argument in its current position? While it does "fix" the issue for callers using positional arguments, it allows callers to use named arguments instead for the remaining two required arguments name and type and omit datacenter. Not sure if thats possible in Python though.

apricote avatar Feb 20 '24 07:02 apricote

To be sure, this is breaking change for the reasons outlined in https://github.com/hetznercloud/hcloud-python/issues/223?

Sort of, adding the star in the argument list will help us to prevent this specific problem in the future indeed. But I am not sure if implementing #223 is a good idea right now, as it will break a LOT of functions.

Is it possible to just define the = None for the datacenter argument in its current position? While it does "fix" the issue for callers using positional arguments, it allows callers to use named arguments instead for the remaining two required arguments name and type and omit datacenter. Not sure if thats possible in Python though.

Python does not allow having optional arguments before required arguments, we MUST move the required name argument before the datacenter one. The order is the breaking change:

-client.primary_ips.create("ipv4", None, "my-ip", assignee_id=12345)
+client.primary_ips.create("ipv4", "my-ip", assignee_id=12345)
-client.primary_ips.create("ipv4", datacenter, "my-ip")
+client.primary_ips.create("ipv4", "my-ip", datacenter)

Using named arguments should be safe with the breaking change:

client.primary_ips.create(type="ipv4", datacenter=None, name="my-ip", assignee_id=12345)

jooola avatar Feb 20 '24 16:02 jooola

This PR has been marked as stale because it has not had recent activity. The bot will close the PR if no further action occurs.

github-actions[bot] avatar Jun 13 '24 12:06 github-actions[bot]