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

Adding MusicBrainz OAuth2 support #2298

Closed
wants to merge 8 commits into from
Closed

Conversation

tigranl
Copy link
Contributor

@tigranl tigranl commented Dec 2, 2016

No description provided.

@sampsyo
Copy link
Member

sampsyo commented Dec 3, 2016

Hi, @tigranl! This is a great start!

There are a few errors here, though—have you tried running the code to see if it works? For example, the exception names like TokenRequestDenied seem to be undefined. These problems look fairly straightforward to fix, at which point we can call the task done. I'm going to add extra time for this task on the GCI site, once I can get it to load (it's moving quite slowly today)…

You can also check out the Travis link above to see our automatic code checker's findings on your code. That can help with both style and correctness.

@@ -28,8 +28,6 @@

SUBMISSION_CHUNK_SIZE = 200
UUID_REGEX = r'^[a-f0-9]{8}(-[a-f0-9]{4}){3}-[a-f0-9]{12}$'
AUTH_ERRORS = (TokenRequestDenied, TokenMissing, VerifierMissing)

Copy link
Member

Choose a reason for hiding this comment

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

The right thing to do here is probably to import these exception names so we can use them, like this: https://github.com/beetbox/beets/blob/master/beetsplug/beatport.py#L26-L27

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about it, but I am using rauth lib and these are from requests_oauthlib.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, of course; that makes sense. So perhaps it would make sense to use the exceptions from rauth instead, or to switch to requests_oauthlib if it's not too different?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was searching for rauth description of exception names, but with no luck. The project's documentation isn't very good. Probably gonna switch to requests_oauthlib.

Copy link
Member

Choose a reason for hiding this comment

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

That sounds great! Sorry for pointing you at the wrong library initially.

@tigranl
Copy link
Contributor Author

tigranl commented Dec 3, 2016

File "/usr/bin/beet", line 11, in <module>
      load_entry_point('beets==1.4.2', 'console_scripts', 'beet')()
  File "/usr/lib/python2.7/site-packages/beets-1.4.2-py2.7.egg/beets/ui/__init__.py", line 1209, in main
    _raw_main(args)
  File "/usr/lib/python2.7/site-packages/beets-1.4.2-py2.7.egg/beets/ui/__init__.py", line 1192, in _raw_main
    subcommands, plugins, lib = _setup(options, lib)
  File "/usr/lib/python2.7/site-packages/beets-1.4.2-py2.7.egg/beets/ui/__init__.py", line 1089, in _setup
    plugins = _load_plugins(config)
  File "/usr/lib/python2.7/site-packages/beets-1.4.2-py2.7.egg/beets/ui/__init__.py", line 1075, in _load_plugins
    plugins.send("pluginload")
  File "/usr/lib/python2.7/site-packages/beets-1.4.2-py2.7.egg/beets/plugins.py", line 451, in send
    for handler in event_handlers()[event]:
  File "/usr/lib/python2.7/site-packages/beets-1.4.2-py2.7.egg/beets/plugins.py", line 434, in event_handlers
    for plugin in find_plugins():
  File "/usr/lib/python2.7/site-packages/beets-1.4.2-py2.7.egg/beets/plugins.py", line 288, in find_plugins
    _instances[cls] = cls()
TypeError: __init__() takes exactly 3 arguments (1 given)

Did I forget to set up some initial settings for plugin scripts?

@sampsyo
Copy link
Member

sampsyo commented Dec 3, 2016

Ah, yes; the __init__ method in plugins is not allowed to take any arguments as you've added here: https://github.com/beetbox/beets/pull/2298/files#diff-771acfd3abde5f720b9b6926c8230bccR58

(As a quick logical check: where would those arguments come from?)

Instead, you might want to move the code for the OAuth dance to its own method. Then, the __init__ method can call that method with the appropriate arguments.

@codecov-io

This comment has been minimized.

@tigranl
Copy link
Contributor Author

tigranl commented Dec 4, 2016

What's the problem with Travis check? 🤔

@sampsyo
Copy link
Member

sampsyo commented Dec 5, 2016

Cool; thanks for updating this!

You can click on the Travis result to see more details. In particular, our style checker has suggestions for you: https://travis-ci.org/beetbox/beets/jobs/181104004#L848

For example, there are a few PEP8 style issues to be corrected. (See the coding conventions on the wiki.) Most of them have to do with indentation—PEP8 mandates four-space indentation—and long lines.

If you like, you can run these checks locally too! Just use pip install flake8 to get the tool. Then type flake8 in your console while in the beets directory to run the checks.

@tigranl
Copy link
Contributor Author

tigranl commented Dec 5, 2016

Yeah, I know about PEP8, sometimes I just forget about it. I checked last commit on my computer using flake8, as you suggested, and it executed without errors. But still can't get the same from Travis.
At least AppVeyor is now green.

@sampsyo
Copy link
Member

sampsyo commented Dec 5, 2016

As usual, it's a good idea to look at the Travis log for the information you need: https://travis-ci.org/beetbox/beets/jobs/181339627#L852

@tigranl
Copy link
Contributor Author

tigranl commented Dec 5, 2016

Oh, missed the second blank line. Sorry, will be more attentive next time!

@sampsyo
Copy link
Member

sampsyo commented Dec 5, 2016

Awesome; thanks! I think we can call the GCI task done if you'd like to submit.

And, if you're interested in taking this one step further, we can look into how to actually use the OAuth credentials to make the requests that this plugin needs to make. This might involve crafting our own API requests instead of going though python-musicbrainzngs. Or we could make progress on alastair/python-musicbrainzngs#199 on the way to resolving alastair/python-musicbrainzngs#89.

@tigranl
Copy link
Contributor Author

tigranl commented Dec 5, 2016

Can we come back to this task on weekend? Now I would like to do some other tasks for Beets or MusicBrainz. Do you have some in mind?

@sampsyo
Copy link
Member

sampsyo commented Dec 5, 2016

Of course!

I don't have any particular preference for any task over any other, but maybe one of these two would be a good fit?
https://codein.withgoogle.com/tasks/5749125039521792/
https://codein.withgoogle.com/tasks/5762730153738240/

@arcresu arcresu changed the title Adding OAuth2 support Adding MusicBrainz OAuth2 support Apr 29, 2019
@jtpavlock
Copy link
Contributor

So is this able to be merged as-is?

@sampsyo
Copy link
Member

sampsyo commented Jul 11, 2020

It looks more or less complete to me, although I have not tried to actually run the code.

@stale
Copy link

stale bot commented Nov 18, 2020

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Nov 18, 2020
@stale stale bot closed this Nov 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants