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

Add more search support - BREAKING CHANGE #5

Merged
merged 6 commits into from
Jul 18, 2021

Conversation

Threpio
Copy link
Contributor

@Threpio Threpio commented Jul 17, 2021

Is required for this issue: spotify-cli #40

Changed the api.Search() function to take in an additional paramater of 'searchType)

FROM:
Search(q string, limit int)
TO:
Search(q, searchType string, limit int)

This will cause all the functionality within spotify-cli that uses the api.Search() to fail as it will require an additional parameter of 'tracks' to beadded


I have realised this included the changes from PR#4 accidentally - Do I need to remove this?

Copy link
Owner

@brianstrauch brianstrauch left a comment

Choose a reason for hiding this comment

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

Thanks for adding the type parameter to Search()! I'm not convinced we need the other two functions though. What do you think?

search.go Show resolved Hide resolved
player.go Outdated Show resolved Hide resolved
@Threpio
Copy link
Contributor Author

Threpio commented Jul 17, 2021

As per your comments on the other PR I will separate these two and remove the other two functions by changing the play() one

@Threpio
Copy link
Contributor Author

Threpio commented Jul 18, 2021

Play() now incorporates all that is required to play a specific album or playlist.

The other two redundant functions have been removed and the other PR closed without merge. This will require a small about of spotify-CLI refactoring

It is not known what happens if you pass a playlist as context_URI but with a song URI that is not in the playlist. If both are passed and the track is found within the playlist then the playlist will be played from that song onwards.

@Threpio
Copy link
Contributor Author

Threpio commented Jul 18, 2021

Screenshot 2021-07-18 at 14 42 04

I think they might need to be split up sadly.

@brianstrauch
Copy link
Owner

brianstrauch commented Jul 18, 2021

I think they might need to be split up sadly.

We can probably do an if statement like:

  1. if contextURI != "", call Play() only with contextURI in the body.
  2. else if len(uris) > 0, call Play() only with uris in the body.
  3. else, call Play() with neither in the body.

@Threpio
Copy link
Contributor Author

Threpio commented Jul 18, 2021

But that logic has to be done within the api.Play() function unless we split it out right? Because where we define the json fields is there

I will try and apply it - I might be a bit confusd though

player.go Outdated Show resolved Hide resolved
player.go Outdated Show resolved Hide resolved
@Threpio
Copy link
Contributor Author

Threpio commented Jul 18, 2021

New push -

  • We need to catch if both are set though?
  • If both are set then we will be returned an error.

Copy link
Owner

@brianstrauch brianstrauch left a comment

Choose a reason for hiding this comment

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

Perfect! Left some picky comments about formatting. This is ready to merge once those get fixed!

player.go Outdated Show resolved Hide resolved
player.go Outdated Show resolved Hide resolved
player.go Outdated Show resolved Hide resolved
player.go Outdated Show resolved Hide resolved
@Threpio
Copy link
Contributor Author

Threpio commented Jul 18, 2021

I know I could have 'commited suggestions' But I wanted to ensure that I had just missed the go fmt command locally and it still worked. All good I believe.

Copy link
Owner

@brianstrauch brianstrauch left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks again for the PR, can't wait to see this in the CLI! I'll release v0.6.0 right now.

@brianstrauch brianstrauch merged commit 629d859 into brianstrauch:master Jul 18, 2021
@Threpio Threpio deleted the add-AlbumSupport branch July 18, 2021 17:24
@Threpio
Copy link
Contributor Author

Threpio commented Jul 18, 2021

Worth noting this was my first open source golang PR.

Glad it worked out on something I will use :)

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.

2 participants