-
Notifications
You must be signed in to change notification settings - Fork 24
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
feature: Nicer handling of disconnects #68
base: master
Are you sure you want to change the base?
Conversation
It would be great if you could rebase this @JJdeVries |
3928953
to
31f4b95
Compare
|
||
await self._async_connect() | ||
|
||
async def async_disconnect(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR contain a breaking change: disconnect
method is replaced by async_disconnect
.
I think it should be properly highlighted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm yes might have been a bit too forward with deleting the previous disconnect method. Although I'm still of the opinion it should be removed, as the lock is definitely needed to avoid nasty errors.
Probably best to re-add the disconnect method including a deprecation warning, to at least give people some time for updating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Version upgrade using py is not mandatory, imho if breaking change is required must be implemented and released without deprecation or work-around, but is important to clarify to the users what is happening. Probably is better to insert in a mayor release as suggested by @kennedyshead because breaking changes are expected.
This one should be a start of a major version upgrade imo! We should create a devel-branch and move this to that one. It requires alfa-testing from different setups so we don't accidentally break someones system. |
@JJdeVries Can you have a quick look here just to be sure I didnt break anything when I fixed the merge-conflict? Then its good to go IMO |
@kennedyshead @Chen-IL I'm not really sure why this was not picked up during the review, as probably the entire file was shown in the diff? Regarding this pull request. I've pushed a commit to also change the line-endings of asuswrt.py to [dos], although probably the result will be that now my name (and this specific commit) will show up in the git blame logging. @Chen-IL please unsure you fix your editor to not overwrite the line-endings on saving the file, but to keep the current configured style. |
Cleanup of some common code between the two connections (ssh and telnet). Mainly reverted the locks and retries to a common class. This also adds locking to the SshConnection as there were some race issues there as well.