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

feat(cli): extract user email from oauth flow #48

Merged
merged 3 commits into from
Feb 26, 2021
Merged

feat(cli): extract user email from oauth flow #48

merged 3 commits into from
Feb 26, 2021

Conversation

y-lakhdar
Copy link
Contributor

Not sure if a decorator would be more appropriate here...

@github-actions
Copy link
Contributor

Pull Request Report

PR Title

✅ Title follows the conventional commit spec.

@olamothe
Copy link
Member

Not sure if a decorator would be more appropriate here...

I don't think so. The decorator pattern can be nice when we know it's going to be re-used in a ton of different command (like, most command 99% will require the user to be connected, so I think it makes sense in that case).

Decorators have the drawback of being harder to read/maintain/not as easy to debug. We should be careful not to be sprinkling them everywhere.

A good ol' function is just generally much more straightforward to reason about.

private async getUserInfo() {
const authenticatedClient = new AuthenticatedClient();
const platformClient = await authenticatedClient.getClient();
await platformClient.initialize();
Copy link
Member

Choose a reason for hiding this comment

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

We could add a method on AuthenticatedClient called getUserInfo that abstract line 91/92 .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. Pretty sure, we will use it elsewhere.
I'm creating a JIRA

@y-lakhdar y-lakhdar merged commit 136d66b into master Feb 26, 2021
@louis-bompart louis-bompart deleted the CDX-91 branch June 7, 2021 13:18
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