Skip to content

Add CLIENT_ID env var for compiling with a client id#125

Closed
sashahilton00 wants to merge 2 commits intomasterfrom
clientid
Closed

Add CLIENT_ID env var for compiling with a client id#125
sashahilton00 wants to merge 2 commits intomasterfrom
clientid

Conversation

@sashahilton00
Copy link
Copy Markdown
Member

Add CLIENT_ID env var for compiling with a client id, remove unnecessary serde_derive. If you can think of a better way than a double match statement, happy to improve this...

@kingosticks
Copy link
Copy Markdown
Contributor

What about let client_key = match option_env!("CLIENT_ID").or(client_id) {...}?

@plietar
Copy link
Copy Markdown
Contributor

plietar commented Feb 7, 2018

Not really happy with this.
I think it's up to the application to hard code values if it wants to, not librespot.

This also adds some potential for unexpected runtime panics if misused.

@sashahilton00
Copy link
Copy Markdown
Member Author

I tried to code it so that it is additive, rather than replacing the current functionality. Isn't a panic still possible in the current implementation in the event that one doesn't pass a client_id to the get_token function?

@kingosticks I did think about that, but I was unsure how match would handle the Option<> that option_env! creates.

@kingosticks
Copy link
Copy Markdown
Contributor

I've not tried it, but if it doesn't evaluate the entire expression before matching then that would be very odd.

@plietar
Copy link
Copy Markdown
Contributor

plietar commented Feb 8, 2018

Since client_id is a &str before your change then no, it's impossible to not provide a value.
With your change one could pass None and expect it to work (ie generates an anonymous token or something).

This is just the wrong place to do this. We don't hardcode anything (even at build time) in the library. End applications are responsible for doing this if they want it.

@kingosticks suggestion is right. You could even get rid of the match with

let client_key = option_env!("CLIENT_ID").or(client_id).expect("No Client ID available");

@sashahilton00
Copy link
Copy Markdown
Member Author

PR updated, but prevaling view seems to be that this isn't the right place/way to implement it, hence closing this PR.

@sashahilton00 sashahilton00 deleted the clientid branch February 8, 2018 08:31
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.

3 participants