-
Notifications
You must be signed in to change notification settings - Fork 2
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 album searching and paging objects as they are different than t… #9
Added album searching and paging objects as they are different than t… #9
Conversation
…racks but same endpoint
brianstrauch/spotify-cli#40 <= for this issue :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but take a look at my comments. I'd also like to see a working PR in the CLI that uses this functionality before I merge this so we're not updating the version multiple times for a single CLI PR.
paging.go
Outdated
// PagingAlbums represents an PagingObject in the spotify API for albums | ||
type PagingAlbums struct { | ||
Albums AlbumPage `json:"albums"` | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A Paging
object already exists (line 5). You can add the Albums
field to that struct and simply use the existing Search()
function instead of creating a new SearchAlbums()
function.
// PagingAlbums represents an PagingObject in the spotify API for albums | |
type PagingAlbums struct { | |
Albums AlbumPage `json:"albums"` | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I should just add a field to the Paging object with something like:
type Paging struct {
Tracks TrackPage `json:"track"``
Albums AlbumPage `json:"album"`
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, exactly. There's unfortunately no way to have multiple json:"item"
tags in Golang, so we can't conform to the Spotify documentation exactly.
album.go
Outdated
func (a *API) GetAlbum(name string) (*Album, error) { | ||
|
||
pagingAlbums, err := a.SearchAlbum(name, "album", 1) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
album := pagingAlbums.Albums.Items | ||
|
||
return album[0], nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd really only like to add functions to this API
that represent endpoints, so let's delete this. This is a convenience function, so it should be added similarly to how this function was added in the CLI: https://github.com/brianstrauch/spotify-cli/blob/def54fb1e6b3ae84292c4ea73d8d32ea00d336f4/internal/common.go#L70-L76.
func (a *API) GetAlbum(name string) (*Album, error) { | |
pagingAlbums, err := a.SearchAlbum(name, "album", 1) | |
if err != nil { | |
return nil, err | |
} | |
album := pagingAlbums.Albums.Items | |
return album[0], nil | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make SearchTrack()
and SearchAlbum()
convenience methods in the CLI (the first already exists; it just needs to be renamed). Then, it would be really slick if we extracted the SearchPlaylist()
functionality into the common.go
file as well, right beneath the other two functions.
As they are different than Tracks but same endpoint.
Had to change the field:
it was of a weird time syntax that I had not seen before and was not automatically transformed
searchAlbums is not hardcoded for type. It probably could be.