Skip to content

#146: Fixed key error for gender when parsing user json#147

Merged
blacklight merged 1 commit into
EbbLabs:masterfrom
mkaufhol:gender-patch-1
Jul 6, 2023
Merged

#146: Fixed key error for gender when parsing user json#147
blacklight merged 1 commit into
EbbLabs:masterfrom
mkaufhol:gender-patch-1

Conversation

@mkaufhol
Copy link
Copy Markdown
Contributor

@mkaufhol mkaufhol commented Jul 5, 2023

Added a patch for the following issue: #146

It seems that the JSON object returned by the Tidal API not longer contains the gender, maybe because it's not set in the user profile. It could make sense to use the get method for other keys, to mitigate this problem for the case, that data is missing in the JSON return object. For example newsletter, dateOfBirth...

@bren-do
Copy link
Copy Markdown

bren-do commented Jul 5, 2023

Can we adopt this approach for most of the JSON fields? How many of these fields, which are currently treated as required, are necessary for the library's logic? Maybe we should treat any fields that can be treated optionally as optional so we don't run into this issue every time Tidal makes a field optional (or removes it).

@mkaufhol
Copy link
Copy Markdown
Contributor Author

mkaufhol commented Jul 5, 2023

Can we adopt this approach for most of the JSON fields? How many of these fields, which are currently treated as required, are necessary for the library's logic? Maybe we should treat any fields that can be treated optionally, as optional, so we don't run into this issue every time Tidal makes a field optional (or removes it).

Totally agree.

@tehkillerbee
Copy link
Copy Markdown
Collaborator

tehkillerbee commented Jul 5, 2023

Can we adopt this approach for most of the JSON fields? How many of these fields, which are currently treated as required, are necessary for the library's logic? Maybe we should treat any fields that can be treated optionally, as optional, so we don't run into this issue every time Tidal makes a field optional (or removes it).

Good idea, if we can determine what fields are optional (i.e. fields that may or may not be filled out by the user). In case a field is removed by TIDAL, it should definitely be removed.

Fields that can be determined as optional (eg. gender) should have a sensible default value. Otherwise, how can we know if the field is actually initialized or not?

@blacklight
Copy link
Copy Markdown
Collaborator

I'm all in for having sensible defaults for API fields that are actually optional. Unfortunately, I'm not sure if anyone here knows the unofficial Tidal API well enough to say which fields are actually optional - or are going to be optional in the near future 😄

If it's ok for you folks I'm going to merge this PR as it would at least fix things on master, and we can then open a separate discussion on figuring out which fields may be likely to break/become optional in the future.

@blacklight blacklight merged commit f45ed75 into EbbLabs:master Jul 6, 2023
@tehkillerbee
Copy link
Copy Markdown
Collaborator

I'm all in for having sensible defaults for API fields that are actually optional. Unfortunately, I'm not sure if anyone here knows the unofficial Tidal API well enough to say which fields are actually optional - or are going to be optional in the near future smile

If it's ok for you folks I'm going to merge this PR as it would at least fix things on master, and we can then open a separate discussion on figuring out which fields may be likely to break/become optional in the future.

Agreed, I have made a new issue for this purpose.

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.

4 participants