hnarayanan / shpotify

A command-line interface to Spotify.
https://harishnarayanan.org/projects/
2.01k stars 153 forks source link

Fix #76 Add Oauth for Search API #81

Closed fonsecapeter closed 6 years ago

fonsecapeter commented 7 years ago

Just hit the token endpoint before hitting the search endpoint

simoheinonen commented 7 years ago

This is very nice!!

On macOS the default base64 doesn't support --wrap=0 option. The GNU one can be installed with brew install coreutils for example. Could consider supporting both somehow though.

$ base64 --help
Usage:  base64 [-hvD] [-b num] [-i in_file] [-o out_file]
  -h, --help     display this message
  -D, --decode   decodes input
  -b, --break    break encoded string into num character lines
  -i, --input    input file (default: "-" for stdin)
  -o, --output   output file (default: "-" for stdout)

$ gbase64 --help
Usage: gbase64 [OPTION]... [FILE]
Base64 encode or decode FILE, or standard input, to standard output.

With no FILE, or when FILE is -, read standard input.

Mandatory arguments to long options are mandatory for short options too.
  -d, --decode          decode data
  -i, --ignore-garbage  when decoding, ignore non-alphabet characters
  -w, --wrap=COLS       wrap encoded lines after COLS character (default 76).
                          Use 0 to disable line wrapping

      --help     display this help and exit
      --version  output version information and exit

The data are encoded as described for the base64 alphabet in RFC 4648.
When decoding, the input may contain newlines in addition to the bytes of
the formal base64 alphabet.  Use --ignore-garbage to attempt to recover
from any other non-alphabet bytes in the encoded stream.

GNU coreutils online help: <http://www.gnu.org/software/coreutils/>
Report base64 translation bugs to <http://translationproject.org/team/>
Full documentation at: <http://www.gnu.org/software/coreutils/base64>
or available locally via: info '(coreutils) base64 invocation'
fonsecapeter commented 7 years ago

Great catch @simoheinonen!

I always forget I have those coreutils, we definitely don't want to add any dependencies. I just pushed a change that uses tr -d "\n" instead (should cover folks who have GNU base64 that defaults to text-wrapping). I tried both with the GNU utils and after a brew uninstall coreutils and everything works fine either way (at least for me)

hnarayanan commented 7 years ago

I am a bit confused. It looks like the discussion is taking place and you're editing your PR's code, but I am not able to see the commits here (as in it still says Commits: 1). How do I see the most recent code so that I can start checking it out?

fonsecapeter commented 7 years ago

Oh that's totally my bad, I usually amend my commit (so it just updates the one every time I push) but I'll stop doing that so it's easier to follow -- I can always squash before the merge if there end up being a bunch of unneeded commits.

Everything under "changes" (the file diff) should be up to date even though its just one commit

hnarayanan commented 7 years ago

Oh, ok. That is not my usual workflow so was confused.

Please, for this PR and contributing to this project, don’t squash your commits. I find it easier to track a changes commit by commit rather than one monolithic patch.

fonsecapeter commented 7 years ago

No worries, will do!

fonsecapeter commented 6 years ago

Hi @hnarayanan -- I'm sure you're super busy, but let me know if there's anything I can do to make it easier to follow my PR. I just want to help ship this so people can back to using the search feature!

I'll definitely stick to just regular commits/no squashing moving forward, but in the meantime I can add some comments to the diff and annotate what's changed if that would help?

The user-community has been super helpful in making sure everything works on vanilla osx, and that seams to be all squared away, so I think the only things outstanding are:

  1. Do you like the idea of having a ~/.shpotify.cfg
    • would you rather this be a pure text file that's parsed instead of sourced?
      • (sourced config=simpler to code, parsed text=maybe simpler for users)
    • would you prefer this file to "auto-touch" if it doesn't exist or let users manually touch the file if they want to use the search feature?
  2. Does the help message look good to you?
    • I tried to stay consistent with the current implementation

Anyway thanks again for making this and keeping it alive!

hnarayanan commented 6 years ago

Thank you!

I've merged this into a separate branch where I can play with it, clean it up if needed and then merge back into master.

hnarayanan commented 6 years ago

And this is now in master, and I will make a new release soon.

This was such an excellent PR that not only added a critical piece of functionality, it really thought through the UI really well and it was a pleasure to use. Thank you very much.

fonsecapeter commented 6 years ago

Awesome thank you! Your project is really well organized and made it easy to jump in