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

Added flags to suppress exceptions from get_entities #32

Merged
merged 4 commits into from Jul 31, 2019

Conversation

@spellew
Copy link
Contributor

commented Jul 14, 2019

No description provided.

@ferbncode

This comment has been minimized.

Copy link

commented Jul 18, 2019

Could you fix the issue on

release_groups = {str(mbid): serialize_release_groups(release_groups[mbid], includes_data[release_groups[mbid].id])
? (It uses mbids, instead of release_group_ids that were returned, which may contain entities that are not present) and throw KeyErrorException. Also, just check for such issues?

One good way to assure everything is working is to add tests which return empty dictionaries with mocked get_entities_by_id function. :)

@ferbncode

This comment has been minimized.

Copy link

commented Jul 18, 2019

I am very very sorry if I am bit late in suggesting it, but you could in fact change the flag name to unknown_entities_for_missing? And return unknown_entities instead in BU? That would be a lot less changes and much improved code (no more tests, for example). What do you think?

@spellew

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

@ferbncode Move the unknown_entity functionality to BU, and return a dictionary containing unknown values here (instead of in CB)?

@spellew

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

If so, we should probably change suppress_no_data_found to something like include_unknown_entities?

@ferbncode

This comment has been minimized.

Copy link

commented Jul 19, 2019

Correct on both points, some flag like unknown_entities_for_missing. I think it would be something very simple to do than do verify that everything is actually working, plus IMHO, it is a much cleaner solution too. :)

@ferbncode
Copy link

left a comment

Very nice work adding tests and separating model definitions to models.py. :)

brainzutils/musicbrainz_db/utils.py Show resolved Hide resolved
brainzutils/musicbrainz_db/utils.py Show resolved Hide resolved
brainzutils/musicbrainz_db/utils.py Show resolved Hide resolved
brainzutils/musicbrainz_db/utils.py Show resolved Hide resolved
@ferbncode

This comment has been minimized.

Copy link

commented Jul 22, 2019

@spellew Understood your point of different functions based on id and gid, fix the rest of the review statements and we can merge this :)

@paramsingh paramsingh merged commit 2ed35d8 into metabrainz:master Jul 31, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.