This repository has been archived by the owner on May 1, 2020. It is now read-only.
LB-331: Case insensitive messybrainz #32
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alastair
reviewed
Mar 20, 2018
messybrainz/data.py
Outdated
| if isinstance(value, str): | ||
| result[key] = value.lower() | ||
| elif isinstace(value, dict): | ||
| result[key] = lower(value) |
There was a problem hiding this comment.
If you're doing these list/dict checks then it'd be good to extend the test to include these things too
alastair
reviewed
Mar 20, 2018
messybrainz/data.py
Outdated
| @@ -144,11 +144,11 @@ def submit_recording(connection, data): | |||
| Returns: | |||
| the Recording MessyBrainz ID of the data | |||
| """ | |||
| data_json = json.dumps(data, sort_keys=True, separators=(',', ':')) | |||
| data_json = _convert_to_messybrainz_json(data) | |||
There was a problem hiding this comment.
I wonder if we should only convert the data to lower-case while computing the sha.
In this example, data_json is now lower-case and we insert it into the database like that. This means that
- it's different from what the user submitted
- if we want to use it to show metadata on the website it's now all lower-case and ugly
as we get the artist name and release data from data, the artist and release data will be original case, but the data in the recording table will be lower-cased
paramsingh
force-pushed
the
case-insensitive-messybrainz
branch
from
4fb6ca7
to
7234e08
Compare
|
Closing in favor of #56 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Lowercase everything before working with the database.
We should probably hold off on deploying this until the data we already have is fixed?