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

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jooola
Copy link
Member

@jooola jooola commented Feb 19, 2024

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

Create a primary ips assigned to a resource without having to pass the datacenter argument.
@jooola
Copy link
Member Author

jooola commented Feb 19, 2024

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?

Copy link

codecov bot commented Feb 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (dd2a521) 92.86% compared to head (8252f5c) 92.86%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #363   +/-   ##
=======================================
  Coverage   92.86%   92.86%           
=======================================
  Files          65       65           
  Lines        2872     2873    +1     
=======================================
+ Hits         2667     2668    +1     
  Misses        205      205           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@apricote
Copy link
Member

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 #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.

@jooola
Copy link
Member Author

jooola commented Feb 20, 2024

To be sure, this is breaking change for the reasons outlined in #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 jooola added this to the v2.0.0 milestone Mar 15, 2024
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