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

Remove tools.checkCredentials() #1555

Merged
merged 1 commit into from
Nov 27, 2019
Merged

Remove tools.checkCredentials() #1555

merged 1 commit into from
Nov 27, 2019

Conversation

fhinkel
Copy link
Contributor

@fhinkel fhinkel commented Nov 27, 2019

We want to remove the nodejs-repo-tools dependency. Doing so step by step.

Drive-by fix: Rename utils to tools for consistency.

We want to remove the nodejs-repo-tools dependency. Doing so step by step.
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 27, 2019
@fhinkel fhinkel added the kokoro:run Add this label to force Kokoro to re-run the tests. label Nov 27, 2019
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Nov 27, 2019
@fhinkel fhinkel merged commit b00d00e into master Nov 27, 2019
@grant
Copy link
Contributor

grant commented Nov 27, 2019

Isn't this introducing a lot of duplicated code?
Why can't we simply extract the method into a helper function that doesn't duplicate code or add SLOCs?

@fhinkel fhinkel deleted the check-credentials branch November 27, 2019 17:57
@fhinkel
Copy link
Contributor Author

fhinkel commented Nov 27, 2019

The point is to get rid of helper methods. Samples need to be self contained. Black box helper methods like checkCredentials lead to unnecessary code execution. As a next step, we need to check for every sample if these checks are necessary.

@grant
Copy link
Contributor

grant commented Nov 27, 2019

From the PR sent, it looked like the motivation was to remove the dependency with nodejs-repo-tools.
I think we could consider simply moving the checkCredentials method to this repo. Or at least state why we aren't doing that in the PR description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants