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

fix: drop almost all user data we don't use. #149

Merged
merged 4 commits into from Jul 10, 2023
Merged

fix: drop almost all user data we don't use. #149

merged 4 commits into from Jul 10, 2023

Conversation

2e0byo
Copy link
Collaborator

@2e0byo 2e0byo commented Jul 8, 2023

Here's a suggestion for handling user data we don't need internally: don't parse it at all.

Reasoning: 99% of the users for python-tidal just want to play music. Whether a user has accepted the eula, or has a facebook id, is irrelevant to them.

Throwing an opinionated PR out there to get opinions.

@BlackLight @tehkillerbee

@tehkillerbee
Copy link
Collaborator

tehkillerbee commented Jul 8, 2023

@2e0byo Agreed, it's been too frequent that tidalapi breaks due to fields being removed by TIDAL. let's stick to supporting the most sensible ones.

I guess the reason for including them is for the sake of API completeness - not because anyone actually needs those fields.

@2e0byo
Copy link
Collaborator Author

2e0byo commented Jul 8, 2023

Good to merge? Or shall we get some opinions. Don't actually want to break anyone's code.

@tehkillerbee
Copy link
Collaborator

tehkillerbee commented Jul 8, 2023

We can wait for @BlackLight and see what he says.

I vote for merging them though. In the unlikely event that the fields are needed by someone, we can easily add them back.

@2e0byo
Copy link
Collaborator Author

2e0byo commented Jul 8, 2023

Agreed on both counts.

So GH auto closes it:

closes #148

@BlackLight
Copy link
Collaborator

Hmm I'm a bit undecided about this.

I mean, those profile fields are subject to a lot of changes as we've noticed lately, and probably all the software that uses python-tidal doesn't use them. But they're still part of the Tidal API and other applications may want to use them.

How about replacing json_obj["attribute"] with json_obj.get("attribute")instead? Things won't break again if some attribute is suddenly removed, but those who use those attributes won't lose access to them - as long as they're aware that those attributes are actually optional and subject to become null at any point in time, of course.

@2e0byo
Copy link
Collaborator Author

2e0byo commented Jul 8, 2023

@BlackLight if we go that way I'd like to do it programmatically, because the might just as easily change name:

for k, v in json_obj.items():
    setattr(self, k, v)

This conflicts a bit with the typehinting, so you want to put them inside a dictionary:

self.other_attrs = {k: v for k, v in json_obj.items() if not hasattr(self, k)}

At which point you land with just passing the json_obj through. Personally I think if we put something on the API we should be saying 'X exists, and if it doesn't it's a bug'. Hence preferring just passing the raw object through to consumers: if tidal rename something tomorrow they can fix it, rather than us ;)

OTOH I'm just explaining my reasoning: I don't really mind and we should def. prefer .get() to breaking.

@BlackLight
Copy link
Collaborator

@2e0byo I see your point, and I definitely don't want to end up in a position where we have to kind of "maintain" attributes that we don't even own upstream.

The point is that we can't even say "if X is not there it's a bug" because we don't know what is going to change upstream and when.

How about the "pass-through" idea though? In this case we would just populate a self.profile_metadatadict attribute or something similar with whatever optional fields come from the API.

It's future proof in both ways - if the library doesn't build business logic on them then it won't break either, and if new fields are added in the future we'll also have them for free. And the non-structured nature of the mapping also makes it clear that it's not something that we want to maintain in our data model - kind of "use at your own risk".

@2e0byo
Copy link
Collaborator Author

2e0byo commented Jul 8, 2023

@BlackLight how should we implement it? I've currently done it by sticking the raw json onto user_data, though I think profile_metadata is a better name. Should we strip keys we pass? Or do any other parsing? (My instinct is no, but there might be good reasons to do so.)

@2e0byo
Copy link
Collaborator Author

2e0byo commented Jul 8, 2023

Regardless of what we do that's def. a better name (I'm rubbish at naming things) so I've updated.

@BlackLight
Copy link
Collaborator

BlackLight commented Jul 8, 2023

@2e0byo the new code looks much better! Just one minor (nitpicking) observation, mostly related to typing - I'd initialize profile_metadata = {}rather than None, and then do a profile_metadata.update(json_obj)rather than an assignment (just in case, to prevent the attribute to change if the upstream dictionary changes). Besides that it looks good to me 👍

@2e0byo
Copy link
Collaborator Author

2e0byo commented Jul 8, 2023

In that case all instances will share the same dict! (Python's mutable default gotcha) But I'll type hints it properly tmrw

@2e0byo 2e0byo merged commit 53e7081 into master Jul 10, 2023
7 checks passed
@2e0byo 2e0byo deleted the fix/user-attrs branch July 10, 2023 17:49
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.

None yet

3 participants