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

allow non-user dependent requests #26

Closed
xxzefgh opened this issue Mar 1, 2018 · 6 comments
Closed

allow non-user dependent requests #26

xxzefgh opened this issue Mar 1, 2018 · 6 comments

Comments

@xxzefgh
Copy link
Contributor

xxzefgh commented Mar 1, 2018

What do you think about this change?
https://github.com/sarkestudio/fitbit-node/blob/master/fitbit-api-client.js#L26-L31
I can make a PR if it makes sense.

@nurseybushc
Copy link
Collaborator

When would you not pass userId?

@xxzefgh
Copy link
Contributor Author

xxzefgh commented Mar 1, 2018

food api has several endpoints where you don't need user
screenshot_20180301-220422

@jmdiego
Copy link
Collaborator

jmdiego commented Mar 1, 2018

What about, instead of requiring userId as false, or setting it as default, just check if it's falsy and do:

getUrl(path, userId){
    return `https://api.fitbit.com/${this.apiVersion}${userId ? `/user/${userId}` : ''}${path}`;
}

@xxzefgh
Copy link
Contributor Author

xxzefgh commented Mar 1, 2018

@jmdiego That would be a breaking change, current implementation automatically inserts - if you don't pass userId, which is useful

@jmdiego
Copy link
Collaborator

jmdiego commented Mar 1, 2018

@thisdotvoid you are right, what about:

getUrl(path, userId){
    let userSubPath = userId === false ? '' : `/user/${userId || '-'}`;
    return `https://api.fitbit.com/${this.apiVersion}${userSubPath}${path}`;
}

Your initial proposal also removed the empty user possibility, so, I guess this would be the most comprehensive solution to handle all three use cases.

@xxzefgh
Copy link
Contributor Author

xxzefgh commented Mar 3, 2018

@jmdiego hmm you are right 🤔 made a pr

jmdiego added a commit that referenced this issue Mar 3, 2018
allow non user-dependent requests #26
@xxzefgh xxzefgh closed this as completed Mar 5, 2018
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

No branches or pull requests

3 participants