Issue 672 #673

PopGoesTheWza · 2019-07-22T20:14:18Z

Fixes #672

npm run test succeeds.
npm run lint succeeds.
Appropriate changes to README are included in PR.

use forEach instead of map when return value is not used (google#614)

Dependencies (google#615)

* dependencies clean-up * types for child_process' spawnSync * types for child_process' spawnSync + options fix * linting

* relative rootDir support * relative rootDir support (untrackedFiles behavior changed) * relative rootDir doc changes

* prettier + sort imports * splitLines types * unused package 'connect' * ucfirst, isOnline types * ellipsize types * redundant package 'fs-copy-file-sync' * removing extra line * // TODO * packages dependencies update * comment fix * fixes * nicer ellipsize typing * better?

* regroup `inquirer` into a single file * fix typo * linting * switch to `find-up` * switch to `find-up` & `strip-bom` * dependencies update * findUp implementation fix * enum accessor fix * fs-extra & typescript dependency fix * linting * dependencies clean-up (again) * non any cast

PopGoesTheWza · 2019-07-22T20:28:49Z

@grant if you prefer, I can change to always use ‘return logError(…)’ for clarity?

grant · 2019-08-01T17:00:06Z

Thanks.
I'd rather see a smaller PRs that don't change functionality for refactoring.
If this breaks something, and we merge other PRs, it will be hard to revert.

For example, import changes should be included in this PR.

Added some comments.

PopGoesTheWza · 2019-08-01T17:20:41Z

Sorry but I unsure I fully understand your comment :)

this PR has no functional changes but only making the use of logError() homogeneous
regarding the import changes, I guess your are refering to how the import statements are sorted on top of each file. I am unsure why it happens (only on this repository!) but from day to day the vsc extension seems to change it sort order.

Just to be sure, package versions upgrade in package.json are best kept in distinct PR (unless when related to the other change in the PR obviously)

grant · 2019-08-01T16:50:07Z

src/apiutils.ts

@@ -51,7 +47,7 @@ export async function getFunctionNames(script: script_v1.Script, scriptId: strin
 // TODO: unnecessary export
 export async function getProjectIdWithErrors() {
  const projectId = await getProjectId(); // will prompt user to set up if required
-  if (!projectId) throw logError(null, ERROR.NO_GCLOUD_PROJECT);


So this throw specifically blocks the return from happening.

I don't know if the caller of getProjectIdWithErrors handles an empty return value, but probably not. I'll still throw here and elsewhere for this PR.

logError() never returns to the caller but always ends the node process.
We could make a choice to:

rename it to something more descriptive like logErrorAndDie ;)

always throw logError

grant · 2019-08-01T16:51:14Z

src/auth.ts

@@ -330,7 +329,7 @@ export async function checkOauthScopes(rc: ClaspToken) {
    await oauthScopesPrompt()
    .then(async (answers) => {
      if (answers.doAuth) {
-        if (!rc.isLocalCreds) return logError(null, ERROR.NO_LOCAL_CREDENTIALS);
+        if (!rc.isLocalCreds) logError(null, ERROR.NO_LOCAL_CREDENTIALS);


Return is needed. Otherwise we try to auth with no credentials.

Same as above

grant · 2019-08-01T16:52:11Z

src/commands/create.ts

@@ -65,10 +64,7 @@ export default async (cmd: { type: string; title: string; parentId: string; root
  spinner.setSpinnerTitle(LOG.CREATE_PROJECT_START(title)).start();
  try {
    const { scriptId } = await getProjectSettings(true);
-    if (scriptId) {
-      logError(null, ERROR.NO_NESTED_PROJECTS);
-      process.exit(1);


Are we sure this isn't needed? I wouldn't add it unless it's necessary.

Same as above.
logError() now has an optional parameter it’ll use with process.exit()

grant · 2019-08-01T16:54:17Z

src/commands/logs.ts

    getProjectSettings().then(projectSettings => {
      console.log(`${LOG.OPEN_LINK(LOG.SCRIPT_LINK(projectSettings.scriptId))}\n`);
      console.log(`${LOG.GET_PROJECT_ID_INSTRUCTIONS}\n`);
      projectIdPrompt()
        .then(answers => {
          projectId = answers.projectId;
          const dotfile = DOTFILE.PROJECT();
-          if (!dotfile) return reject(logError(null, ERROR.SETTINGS_DNE));
+          if (!dotfile) logError(null, ERROR.SETTINGS_DNE);
          dotfile


I'm pretty sure this will fail. Is this on purpose?

I don't know if all the cases are handled. Maybe we can explain the way this works in a comment?

Since logError always end by calling process.exit() it should not change anything.

I opened this issue/PR because in many places of the source, the usage of logError() was (misleadingly) hinting that it would return and resume execution (which doesn’t”t occur ever)

grant · 2019-08-06T15:09:42Z

Thanks.
I don't know why I was so confused by the return issue.

PopGoesTheWza · 2019-08-06T15:24:46Z

I initiated this PR just because of this, use of logError() was inducing confusion.
And after your review, I now wonder if renaming logError to logErrorAndExitProcess wouldn't help preventing any more confusion in the future.

PopGoesTheWza and others added 21 commits May 14, 2019 00:03

Merge pull request #17 from google/master

5bdc1b4

use forEach instead of map when return value is not used (google#614)

Merge pull request #18 from google/master

ed24bb7

Dependencies (google#615)

2.1.0

807729c

Low impact changes (google#619)

83b9e78

* dependencies clean-up * types for child_process' spawnSync * types for child_process' spawnSync + options fix * linting

Relative rootdir (google#620)

a89ab84

* relative rootDir support * relative rootDir support (untrackedFiles behavior changed) * relative rootDir doc changes

Merge branch 'master' of https://github.com/google/clasp

4829c18

Merge branch 'master' of https://github.com/google/clasp

4b77408

Merge branch 'master' of https://github.com/google/clasp

cd4b7cc

Merge branch 'master' of https://github.com/google/clasp

5279445

Merge branch 'master' of https://github.com/google/clasp

4b0fe1c

Merge branch 'master' of https://github.com/google/clasp

6aca82a

Merge branch 'master' of https://github.com/google/clasp

ec9f3e8

Merge branch 'master' of https://github.com/google/clasp

81d3792

Merge branch 'master' of https://github.com/google/clasp

9a911f5

Merge branch 'master' of https://github.com/google/clasp

4cd43a9

Merge branch 'master' of https://github.com/google/clasp

8018780

Merge branch 'master' of https://github.com/google/clasp

f3706ea

Merge branch 'master' of https://github.com/google/clasp

f468920

refactoring logError

7f6c6a0

googlebot added the cla: yes label Jul 22, 2019

grant reviewed Aug 3, 2019

View reviewed changes

grant approved these changes Aug 6, 2019

View reviewed changes

grant merged commit ee16b6d into google:master Aug 6, 2019

PopGoesTheWza deleted the issue-672 branch August 6, 2019 15:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 672 #673

Issue 672 #673

PopGoesTheWza commented Jul 22, 2019

PopGoesTheWza commented Jul 22, 2019

grant commented Aug 1, 2019 •

edited

PopGoesTheWza commented Aug 1, 2019

grant Aug 1, 2019

PopGoesTheWza Aug 3, 2019

grant Aug 1, 2019

PopGoesTheWza Aug 3, 2019

grant Aug 1, 2019

PopGoesTheWza Aug 3, 2019

grant Aug 1, 2019

PopGoesTheWza Aug 3, 2019

grant commented Aug 6, 2019

PopGoesTheWza commented Aug 6, 2019

Issue 672 #673

Issue 672 #673

Conversation

PopGoesTheWza commented Jul 22, 2019

PopGoesTheWza commented Jul 22, 2019

grant commented Aug 1, 2019 • edited

PopGoesTheWza commented Aug 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grant commented Aug 6, 2019

PopGoesTheWza commented Aug 6, 2019

grant commented Aug 1, 2019 •

edited