Add 'play --playlist' support. Issue #27 #41

Threpio · 2021-07-20T12:16:10Z

Fixes issue #27

Upgraded root spotify module from v0.5.0 => v0.6.0.
Added --playlist flag to the play function that takes a playlist name and plays it.

Tests have been updated to reflect upgrade from spotify v0.5.0 -> v0.6.0
New tests required for additional features.

brianstrauch

Looks good, although I think we should change the Play() function to better support playing albums... see my comment below. (And then you could easily sneak the --album flag into this PR!)

internal/mock_api.go

brianstrauch · 2021-07-20T15:25:36Z

internal/p/p.go


 	return cmd
 }

-func p(api internal.APIInterface, query, deviceID string) (string, error) {
+func p(api internal.APIInterface, query, contextQuery, deviceID string) (string, error) {


Looks like the logic here needs to be fixed to use contextQuery

internal/p/p_test.go

brianstrauch · 2021-07-20T15:39:31Z

internal/play/play.go


 	return cmd
 }

-func Play(api internal.APIInterface, query, deviceID string) (string, error) {
+func Play(api internal.APIInterface, query, contextQuery , deviceID string) (string, error) {


I think we should pass query and queryType here, where queryType is "track", "album", or "playlist". Then we can switch on queryType to determine if we should do internal.Search() or a linear search through all of the user's playlists.

An internal.Search() currently returns a *spotify.track not a *spotify.album/playlist

So we need to modify that to allow for a return on an album.

If we do it for albums then the logic I have currently done for finding playlists can be overhauled aswel

I suppose we should just get rid of internal.Search(), and instead call the API directly and return the PagingObject. Playing albums should have the same logic as playing tracks, just a different queryType. I like your current logic for playlists though. We should keep that.

The logic for finding a playlist by name already exists for spotify playlist show, so we might be able to steal that code and use it here

internal/play/play_test.go

Co-authored-by: Brian Strauch <bstrauch24@gmail.com>

…es PR on spotify

Threpio · 2021-07-21T14:01:02Z

@brianstrauch - PR for commands that would show how we can use the Spotify library.
I will split the commands up to do searchPlaylists, searchAlbums and searchTracks

The tests pass but I am not sure they are done correctly

brianstrauch

Looks awesome! I was just thinking about how much I needed these changes earlier today 😄

Only two changes left to make before we merge:

We should be passing flag values directly into the p() and Play() functions, so we can unit test all of the logic with the mock API.
Logic is duplicated between the p() and Play() functions, let's only write it once, in Play(). p() can call Play()

internal/p/p.go

internal/p/p_test.go

internal/play/play.go

internal/play/play_test.go

internal/errors.go

Co-authored-by: Brian Strauch <bstrauch24@gmail.com>

brianstrauch

FYI: I'm merging brianstrauch/spotify#9 tonight and releasing v0.7.0... just to keep things moving. Just a few fixes left on this PR.

internal/p/p_test.go

internal/errors.go

brianstrauch · 2021-07-25T22:05:52Z

Hey @Threpio, how's this coming along?

Threpio · 2021-07-26T12:18:52Z

Was away for the weekend - apologies.

Will have another update by this evening (CET)

Co-authored-by: Brian Strauch <bstrauch24@gmail.com>

…/spotify-cli into playlist-brianstrauch#27 # Conflicts: # internal/p/p.go

…/Threpio/spotify-cli into playlist-brianstrauch#27" This reverts commit 6fdf6dd, reversing changes made to bdabdb5.

…/spotify-cli into playlist-brianstrauch#27

Threpio · 2021-07-26T14:33:26Z

I won't be able to complete this for a week or so because of prior commitments - If you want to run with it then fire away!

Threpio · 2021-07-30T13:12:38Z

Done a couple of tests locally and it seems that all the new functionality is done correctly - Still intermittently getting the previous song shown when selecting a playlist...

EDIT:
I would like for this to be a squash commit considering I mucked the git up a whole load.

brianstrauch · 2021-07-30T15:17:31Z

@Threpio Thanks, I'll take a look later today! ... and no worries, squash is the default for this repo 😄

brianstrauch

Looks great, I'll make a few touch-ups and try to fix the issue you mentioned above! Thanks again for the PR!

Threpio added 2 commits July 20, 2021 13:32

Tests broken but playlist functionality added

f9a2f67

Fixed tests but need to make new tests for 'play-playlist'

0e9fa7e

brianstrauch reviewed Jul 20, 2021

View reviewed changes

Threpio and others added 4 commits July 20, 2021 17:57

Update internal/play/play_test.go

dd05276

Co-authored-by: Brian Strauch <bstrauch24@gmail.com>

Update internal/p/p_test.go

2d95663

Co-authored-by: Brian Strauch <bstrauch24@gmail.com>

Syntax updates to mock_api.go

00d207b

Co-authored-by: Brian Strauch <bstrauch24@gmail.com>

Added switch case to play functions and adapted to queryType - Requir…

8f9bf24

…es PR on spotify

brianstrauch requested changes Jul 21, 2021

View reviewed changes

Threpio and others added 9 commits July 21, 2021 17:58

Rename errors as per suggestion

407d949

Co-authored-by: Brian Strauch <bstrauch24@gmail.com>

Fixing Tests

7c5b765

Co-authored-by: Brian Strauch <bstrauch24@gmail.com>

Fixing Tests

4548419

Co-authored-by: Brian Strauch <bstrauch24@gmail.com>

Fixing Tests

888467e

Co-authored-by: Brian Strauch <bstrauch24@gmail.com>

Syntax cleanup

ac83dbb

Co-authored-by: Brian Strauch <bstrauch24@gmail.com>

Syntax cleanup

e4f3927

Co-authored-by: Brian Strauch <bstrauch24@gmail.com>

Fixing Tests

2ab9e35

Co-authored-by: Brian Strauch <bstrauch24@gmail.com>

Fixing command flags as per suggestion

3e2f9fc

Co-authored-by: Brian Strauch <bstrauch24@gmail.com>

Update internal/p/p_test.go

1849cf5

Co-authored-by: Brian Strauch <bstrauch24@gmail.com>

brianstrauch reviewed Jul 23, 2021

View reviewed changes

internal/p/p_test.go Outdated Show resolved Hide resolved

internal/errors.go Outdated Show resolved Hide resolved

Threpio and others added 9 commits July 26, 2021 14:20

Syntax Updates

b67dd0c

Co-authored-by: Brian Strauch <bstrauch24@gmail.com>

Update internal/errors.go

fb37948

Co-authored-by: Brian Strauch <bstrauch24@gmail.com>

Updated p to better reflect play

bdabdb5

Merge branch 'playlist-brianstrauch#27' of https://github.com/Threpio…

6fdf6dd

…/spotify-cli into playlist-brianstrauch#27 # Conflicts: # internal/p/p.go

Revert "Merge branch 'playlist-brianstrauch#27' of https://github.com…

3a9501e

…/Threpio/spotify-cli into playlist-brianstrauch#27" This reverts commit 6fdf6dd, reversing changes made to bdabdb5.

Merge branch 'brianstrauch:master' into playlist-brianstrauch#27

bbeacc2

updated to 0.7.0 -> Failed with some merging so git is messy

76e95c7

Sorted out errors again

21c5988

Merge branch 'playlist-brianstrauch#27' of https://github.com/Threpio…

5aa1acd

…/spotify-cli into playlist-brianstrauch#27

Threpio added 2 commits July 26, 2021 15:55

More sorting out of play functions

3df19c2

Running go fmt

1957117

Threpio marked this pull request as ready for review July 30, 2021 13:12

Forgot contextQuery Logic for p() function

23eda79

brianstrauch approved these changes Aug 1, 2021

View reviewed changes

brianstrauch merged commit a48566e into brianstrauch:master Aug 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'play --playlist' support. Issue #27 #41

Add 'play --playlist' support. Issue #27 #41

Threpio commented Jul 20, 2021

brianstrauch left a comment

brianstrauch Jul 20, 2021

brianstrauch Jul 20, 2021

Threpio Jul 20, 2021

brianstrauch Jul 20, 2021

brianstrauch Jul 20, 2021

Threpio commented Jul 21, 2021 •

edited

Loading

brianstrauch left a comment

brianstrauch left a comment

brianstrauch commented Jul 25, 2021

Threpio commented Jul 26, 2021

Threpio commented Jul 26, 2021

Threpio commented Jul 30, 2021 •

edited

Loading

brianstrauch commented Jul 30, 2021

brianstrauch left a comment

Add 'play --playlist' support. Issue #27 #41

Add 'play --playlist' support. Issue #27 #41

Conversation

Threpio commented Jul 20, 2021

brianstrauch left a comment

Choose a reason for hiding this comment

brianstrauch Jul 20, 2021

Choose a reason for hiding this comment

brianstrauch Jul 20, 2021

Choose a reason for hiding this comment

Threpio Jul 20, 2021

Choose a reason for hiding this comment

brianstrauch Jul 20, 2021

Choose a reason for hiding this comment

brianstrauch Jul 20, 2021

Choose a reason for hiding this comment

Threpio commented Jul 21, 2021 • edited Loading

brianstrauch left a comment

Choose a reason for hiding this comment

brianstrauch left a comment

Choose a reason for hiding this comment

brianstrauch commented Jul 25, 2021

Threpio commented Jul 26, 2021

Threpio commented Jul 26, 2021

Threpio commented Jul 30, 2021 • edited Loading

brianstrauch commented Jul 30, 2021

brianstrauch left a comment

Choose a reason for hiding this comment

Threpio commented Jul 21, 2021 •

edited

Loading

Threpio commented Jul 30, 2021 •

edited

Loading