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

Fetch original_year from master releases for Discogs (#1122) #2893

Merged
merged 3 commits into from
May 2, 2018

Conversation

dbogdanov
Copy link
Contributor

This adds an additional Discogs API call from inside get_album_info() via a very basic fetch method (it only returns year). This may be expanded in the future to fetch other fields from the master release (genres, styles, etc.) if necessary.

Assume that original_year equals to year for releases without a master release. If parsing the original year in the master release fails, the resulting value is kept as None.

I am not sure how to update tests, because the API call is inside get_album_info. The functionality is so basic that possibly it is not necessary (and other API calls aren't tested as well).

This adds an additional Discogs API call inside get_album_info().
Assume that original_year equals to year for releases without a
master release.
@sampsyo
Copy link
Member

sampsyo commented Apr 29, 2018

Looks great! This information will be useful to have.

I notice that we now have some similar-looking exception-handling code for the calls into the Discogs API. (The handlers for DiscogsAPIError and CONNECTION_ERRORS are very similar between the Master and Release fetching code, for example.) Would it make sense to refactor that code so it can be shared between the different resource retrievals?

@dbogdanov
Copy link
Contributor Author

May be, but it may also complicate the code. The code is similar for candidates(), album_for_id() and get_master_year() methods. The return values, retry method and log messages are different though. If there will be more methods like this than yes.

@sampsyo
Copy link
Member

sampsyo commented Apr 29, 2018

OK, cool. It's only worth pursuing if it can be done in a way that makes the code simpler, not more complex. Other than that, I think all we need is a changelog entry here.

@sampsyo
Copy link
Member

sampsyo commented May 2, 2018

Perfect. Thanks again! ✨

@sampsyo sampsyo merged commit 0712076 into beetbox:master May 2, 2018
@sampsyo
Copy link
Member

sampsyo commented May 2, 2018

...and thanks for all your contributions over the last week or two. I’ve invited you as an organization member, so you should now be able to commit directly. Of course, pull requests are always still an option as a way to get code reviews!

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