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

Update dialup profile #221

Merged
merged 2 commits into from
Aug 19, 2024
Merged

Conversation

timvlaer
Copy link
Contributor

@timvlaer timvlaer commented Jul 19, 2024

While working on the api for dialup profiles, I noticed a few things:

  • it looks like you can't really control the behaviour of SetDefault when creating a profile. It looks like newly created profiles always become the default, irregardless of the SetDefault parameter value. I removed the parameter from the function.
  • When creating a profile without dialup_number (DialupNum), the request failed with error 100001. It looks to me that this field is not optional in my setup... I didn't change this in the code as the example script doesn't have this param so I guess it might work in some cases without dialup_number.
  • FYI, I couldn't remove the default profile, which has the ReadOnly set to 2 (instead of 0 for manually created profiles)

I tested these things against the api of the Huawei E5576-320, so it might be different on other devices.

@Salamek Salamek self-assigned this Jul 23, 2024
@Salamek Salamek added the enhancement New feature or request label Jul 23, 2024
@Salamek
Copy link
Owner

Salamek commented Jul 30, 2024

@timvlaer ping

@timvlaer
Copy link
Contributor Author

@Salamek this pr is ready for review. I don’t plan any additions.

) -> SetResponseType:
return self._session.post_set('dialup/profiles', {
'SetDefault': 1 if is_default else 0,
'SetDefault': 0,
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure if it is a good idea to remove is_default parameter and its code, while it does not work for you as expected, it may work for someone else. Also it is BC because someone may call this function with is_default= kwargs or have positional argument there.
We could deprecate this parameter with warning and remove it next minor version but it think that just keeping it there is much less work :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I’ll revert it

@Salamek
Copy link
Owner

Salamek commented Jul 31, 2024

@timvlaer ah sorry my bad, i did not post the review...

@timvlaer
Copy link
Contributor Author

My apologies, i am abroad right now without computer so I’ll update this PR as soon as i am back home. That will be the 19th of August.

@Salamek
Copy link
Owner

Salamek commented Jul 31, 2024

@timvlaer that is completely fine ;-) Enjoy your self.

@timvlaer
Copy link
Contributor Author

Reverted the is_default param

@Salamek Salamek merged commit 8c741f8 into Salamek:master Aug 19, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants