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

remove kwargs client #2244

Merged
merged 1 commit into from
Jul 22, 2024
Merged

remove kwargs client #2244

merged 1 commit into from
Jul 22, 2024

Conversation

janiversen
Copy link
Collaborator

@janiversen janiversen commented Jul 19, 2024

kwargs is gone from all clients !!!

Hopefully that will reduce the number of questions about wrong parameters.

@janiversen janiversen marked this pull request as draft July 20, 2024 07:05
@janiversen janiversen force-pushed the kwargs5 branch 2 times, most recently from 72a7175 to f5621bc Compare July 20, 2024 11:59
@janiversen janiversen marked this pull request as ready for review July 20, 2024 12:00
@janiversen janiversen force-pushed the kwargs5 branch 4 times, most recently from 5b3d564 to 086f166 Compare July 22, 2024 09:49
@janiversen janiversen merged commit f0d6842 into dev Jul 22, 2024
1 check passed
@janiversen janiversen deleted the kwargs5 branch July 22, 2024 19:53
Comment on lines -60 to +64
port: int = 502,
framer: FramerType = FramerType.SOCKET,
port: int = 502,
Copy link

Choose a reason for hiding this comment

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

For reference: this breaks the interface for those who used port as a positional argument

Copy link
Collaborator Author

@janiversen janiversen Aug 1, 2024

Choose a reason for hiding this comment

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

Yes, as noted in API changes.

But it still allows using port as a positional parameter, we just changed the order! using optional parameters as positional is very dangerous since optional parameters can change position without notice.

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