-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Implement "Create new release on musicbrainz" feature #5300
base: master
Are you sure you want to change the base?
Implement "Create new release on musicbrainz" feature #5300
Conversation
f499389
to
ffcce62
Compare
0ab0a85
to
096356e
Compare
Hey @doronbehar, If you're around, have a look at this. Maybe I can remove the "Open with Picard" option now that this here is implemented? |
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.
Thanks for the PR! I've made a couple comments but I'll also note that a fair amount of the changes in this PR are fairly boilerplate HTTP webserver stuff. Have you considered using something like simple-http-server or another package to deal with this? It would reduce the number of lines in the PR and make it easier to change in the future.
beetsplug/mbsubmit.py
Outdated
formdata: dict | ||
""" | ||
Form data to be submitted to MB. | ||
""" | ||
|
||
result_release_mbid: Optional[str] = None | ||
""" | ||
Contains the release ID returned by MusicBrainz after the release was created. | ||
""" | ||
|
||
browser_opened: bool = False | ||
""" | ||
True when the user has opened the link for this task in the browser. | ||
""" |
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.
Docstring comments go above variables. It's only classes and functions that have them below the definition. Confusing, I know
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.
Do they? See https://peps.python.org/pep-0258/#attribute-docstrings... also, PyCharm correctly picks up the docstrings this way.
docs/plugins/mbsubmit.rst
Outdated
|
||
.. code-block:: console | ||
|
||
$ pip install beets[mbsubmit] |
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.
With the switch to poetry, this is now outdated. Should be changed to the poetry equivalent.
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.
Is it?
https://github.com/beetbox/beets/blob/master/docs/plugins/fetchart.rst?plain=1#L12
I can add quotation marks for consistency.
I thought about not pulling int too many new dependencies. I can also use flask instead, since that's already used by a couple other plugins...? |
I don't think many dependencies is an issue that is too pressing. If it makes the code easier to read and maintain, that wins out over another dependency in my book. If flask is used by other plugins though, by all means use it! |
Thank you for the review! It's now much cleaner and shorter with flask. |
Hey! Personally I won't mind it getting removed, as I'd definitely use this functionality once it will land! It also makes sense to me from a general point of view to remove it, but that needs an approval from the maintainers of beets. Thanks for working on this feature! |
2486114
to
420178a
Compare
|
||
.. code-block:: console | ||
|
||
$ pip install "beets[mbsubmit]" |
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.
We're now recommending that people install and manage beets with poetry, so if you wouldn't mind changing this, it should be poetry install beets --extras mbsubmit
.
formdata: Dict[str, str] | ||
""" | ||
Form data to be submitted to MB. | ||
""" | ||
|
||
browser_opened: bool = False | ||
""" | ||
True when the user has opened the link for this task in the browser. | ||
""" | ||
|
||
result_release_mbid: Optional[str] = None | ||
""" | ||
Contains the release ID returned by MusicBrainz after the release was created. | ||
""" |
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.
Comments go above variables, please.
def _wait_for_condition(self, condition: Callable): | ||
t = threading.current_thread() | ||
while not condition(): | ||
time.sleep(0.5) |
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.
Is this necessary? This is a blocking call on the other thread, so is there any penalty to removing it?
Description
Closes #1866.
Implement support to create new releases on musicbrainz from unmatched albums directly.
Screencast.from.2024-06-12.23-37-41.webm
See #5299.
See documentation and comments for details.
If no unmatched album is available for testing, set configuration mbsubmit.threshold to 'strong' and run import with --timid to select the new option.
To Do
docs/
to describe it.)docs/changelog.rst
to the bottom of one of the lists near the top of the document.)