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

extract additional album info #60

Merged
merged 4 commits into from
Sep 4, 2024

Conversation

ChinoUkaegbu
Copy link
Contributor

Fixes #32

In addition to returning the album labels, also returns total number of tracks in the album as well as the album type because why not?

Also turns out Spotify's API also lets you get genres for albums which I think is more specific than Artist's genres which is what was implemented. Unfortunately for all the albums I tried it with, it just always returned an empty list (even when I tried on their website) so either it's a bug on Spotify or idk

Which sucks because it would also have cut down the amount of requests we were doing since we won't have needed to query the artist's endpoints too.

@ChinoUkaegbu
Copy link
Contributor Author

Also lmk if I should get rid of the extra fields since only label was initially asked but I doubt the additional fields are adding much delays since it's all retrieved either way.

@pavelkomarov
Copy link
Owner

Regarding empty lists for genres: Their Web API is definitely imperfect! I discovered an issue with it the other day #56. You may be able to open a forum post and get it addressed.

@pavelkomarov
Copy link
Owner

pavelkomarov commented Sep 3, 2024

I feel like there's still potentially scope in this issue to make certain fields selectable, but then again that may be total overkill; maybe we should just give all the fields all the time and not worry the user with a choice they mostly don't care about. It's easier to wait a couple more seconds and ignore fields you don't need than to think through which ones you do or discover some you wanted actually weren't fetched because they were part of another group you deselected.

Philosophy of simplicity. It's why I got rid of the paginator. Related story: I once had an internship on Microsoft's Outlook team and discovered a bunch of PMs trying to think through new, additional features for the app. I told them it was far too cluttered and they should be looking for ways to remove features. Didn't win me friends, but I'm proud of myself for saying what needed to be said.

@gregsadetsky, do you have an opinion? We can always merge this and make selectability its own follow-on issue, if my instincts are wrong and it turns out to be in demand.

pavelkomarov
pavelkomarov previously approved these changes Sep 3, 2024
Copy link
Owner

@pavelkomarov pavelkomarov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it works, though I haven't pulled your fork, served it, and tried exporting stuff to check.

exportify.js Outdated Show resolved Hide resolved
exportify.js Outdated Show resolved Hide resolved
@pavelkomarov pavelkomarov dismissed their stale review September 4, 2024 19:10

I just tried, and it's failing

@pavelkomarov
Copy link
Owner

pavelkomarov commented Sep 4, 2024

I just pulled your branch and tried to Export All, and I'm getting rate limiting errors and undefined is not an object errors.

I think this issue is both genre_promise and album_promise are solely dependent upon data_promise, so they're happening concurrently. You need a Promise.all([data_promise, genre_promise]).

I would hop on your branch and just fix it, but I don't think I have permissions on your fork to do that. Maybe I can merge to a feature branch on my own repo and then merge that? Or maybe there's another way https://tighten.com/insights/adding-commits-to-a-pull-request/

@pavelkomarov
Copy link
Owner

pavelkomarov commented Sep 4, 2024

I've reported the empty list issue on the Spotify dev forum. According to a months-old forum post that I link therein, the workaround is fetching genres from artists. Unclear whether that is intentional behavior.

…ed to see those while pulling them out of the data with shift() so I can always use the id in the first column rather than accessing particular columns, got rid of album info I deem mostly useless, use user id instead of uri in the output now
@pavelkomarov pavelkomarov merged commit ec1d7c7 into pavelkomarov:master Sep 4, 2024
@ChinoUkaegbu
Copy link
Contributor Author

I just pulled your branch and tried to Export All, and I'm getting rate limiting errors and undefined is not an object errors.

I think this issue is both genre_promise and album_promise are solely dependent upon data_promise, so they're happening concurrently. You need a Promise.all([data_promise, genre_promise]).

I would hop on your branch and just fix it, but I don't think I have permissions on your fork to do that. Maybe I can merge to a feature branch on my own repo and then merge that? Or maybe there's another way https://tighten.com/insights/adding-commits-to-a-pull-request/

my bad, i was very much asleep during this but glad you figured it out! i can add more comments to #62 but i do agree with the simplicity thing. with the way it's structured we already get most of the data in the first wave. the whole selection thing is for performance issues but the only way it makes an improvement is if users do not select genre and record label to be exported (i'm terrible at probability but it does seem like unlikely fields to deselect for no reason...unless you somehow include that deselecting those particular fields could improve performance in which case there's some sort of incentive to do so). from a (solely) usability perspective, i think the option to select fields to include is good though but might be worth it to have this in discussions.

@gregsadetsky
Copy link

gregsadetsky commented Sep 22, 2024

sorry for not chiming in earlier, had some crazy weeks. thanks a LOT! I see now the labels in the csv output and that's fantastic! thank youuu!!

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.

add gear / config back on exportify.net?
3 participants