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

Moved LinkLogin and futures to login_oauth. #303

Merged
merged 2 commits into from
Nov 27, 2024

Conversation

exislow
Copy link
Contributor

@exislow exislow commented Nov 13, 2024

No description provided.

@@ -644,7 +646,7 @@ def load_session_from_file(self, session_file: Path):

return self.load_oauth_session(**args)

def _login_with_link(self) -> Tuple[LinkLogin, concurrent.futures.Future[Any]]:
def _login_with_link(self) -> JsonObj:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This method should not be a private method if 3rd party libs plan to access it directly. I also suggest adding doxygen for the same reason as above.

It should also be renamed, since its purpose will have changed significantly. Perhaps something like oauth_request_device_auth()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are totally right. Excuse me this sloppy PR. I will do a proper refactoring and re-submit this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No problem at all :)

… considered private. `LinkLogin` is used instead of `JsonObj`. Docstrings are added.
@exislow
Copy link
Contributor Author

exislow commented Nov 14, 2024

I got a proper update. Would be very happy about your opinion. You can see here, how I would use it: https://github.com/exislow/tidal-dl-ng/blob/f2b3cc2505f4c64f09efacd775de3390ae204be0/tidal_dl_ng/gui.py#L134-L155

@tehkillerbee
Copy link
Collaborator

tehkillerbee commented Nov 27, 2024

I got a proper update. Would be very happy about your opinion. You can see here, how I would use it: https://github.com/exislow/tidal-dl-ng/blob/f2b3cc2505f4c64f09efacd775de3390ae204be0/tidal_dl_ng/gui.py#L134-L155

Thanks for the refactor, this is a neat way to perform the login. I was never happy with the hacks needed in mopidy-tidal so this is a good example of how it can be done in a different way.

Works as expected, thanks for the added docstring btw.. Merging... 👍

@tehkillerbee tehkillerbee merged commit 9c451c7 into tamland:master Nov 27, 2024
5 checks passed
@exislow
Copy link
Contributor Author

exislow commented Nov 28, 2024

I am glad you liked it! Really like how I easily we can work together on this project. Thank you.

@exislow exislow deleted the 302-return-raw-values branch December 16, 2024 15:01
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