fix(cli): Scaffold errors if file name exists #88

stevelikesmusic · 2019-10-24T07:11:42Z

Fixes: #17

This prevents scaffold from overwriting files if they exist. The option to force an overwrite is also now built in.

Also, ports scaffold to oclif.

packages/cli/src/oclif/commands/scaffold.js

xavdid

some stylistic things to keep it in line with our newer CLI code. otherwise looks decent though. thanks for grabbing!

packages/cli/src/oclif/commands/scaffold.js

eliangcs

I did a sanity test and it works! I didn't look closely because I believe @xavdid already did.

…or message

stevelikesmusic · 2019-10-28T06:36:53Z

@xavdid I think it's ready for another pass. Thanks!

stevelikesmusic · 2019-10-28T06:38:03Z

packages/cli/src/oclif/commands/scaffold.js

+function createTemplateContext(type, name) {
+  const contextKey = snakeCase(name);
+
+  // where will we create/required the new file?
+  const destMap = {
+    resource: `resources/${contextKey}`,
+    trigger: `triggers/${contextKey}`,
+    search: `searches/${contextKey}`,
+    create: `creates/${contextKey}`
+  };
+
+  return {
+    CAMEL: camelCase(name),
+    KEY: contextKey,
+    NOUN: _.capitalize(name),
+    LOWER_NOUN: name.toLowerCase(),
+    INPUT_FIELDS: '',
+    TYPE: type,
+    TYPE_PLURAL: type === 'search' ? `${type}es` : `${type}s`,
+    // resources need an extra line for tests to "just run"
+    MAYBE_RESOURCE: type === 'resource' ? 'list.' : '',
+    defaultDest: destMap[type]
+  };
+}
+
+function getTemplatePath(type) {
+  return path.join(__dirname, '../../..', `scaffold/${type}.template.js`);
+}


I liked the feel of pulling these functions out of the class. They're not exactly utils worthy of their own file, but could be class methods if we really wanted.

yeah, either way! I think it's reasonable to bundle them all together. Typically we could test them, though these are simple enough we probably won't bother.

stevelikesmusic · 2019-10-28T06:40:10Z

packages/cli/src/utils/string.js

@@ -0,0 +1,13 @@
+function splitFileFromPath(filePath, extension = 'js') {


This function suits our purpose well for scaffold. I left out considering the extension being passed in for now (ex. src/triggers/foo.js since we'd also need to account for foo.test.js)

stevelikesmusic · 2019-10-28T06:42:34Z

packages/cli/src/oclif/commands/scaffold.js

+      // this is very dumb and will definitely break, it inserts lines of code
+      // we should look at jscodeshift or friends to do this instead
+
+      // insert Resource = require() line at top
+      const varName = `${templateContext.CAMEL}${camelCase(type)}`;
+      const importerLine = `const ${varName} = require('./${dest}');`;
+      lines.splice(0, 0, importerLine);
+
+      // insert '[Resource.key]: Resource,' after 'resources:' line
+      const injectAfter = `${typeMap[type]}: {`;
+      const injectorLine = `[${varName}.key]: ${varName},`;
+      const linesDefIndex = lines.findIndex(line => line.endsWith(injectAfter));
+
+      if (linesDefIndex === -1) {
+        this.stopSpinner(false);
+        return this.error(
+          [
+            `\n${colors.bold(
+              `Oops, we could not reliably rewrite your ${entry}.`
+            )} Please add:`,
+            ` * \`${importerLine}\` to the top`,
+            ` * \`${injectAfter} ${injectorLine} },\` in your app definition`
+          ].join('\n')
+        );
+      }
+
+      lines.splice(linesDefIndex + 1, 0, '    ' + injectorLine);


I almost think this section could be its own function so it's even easier to swap in an AST approach eventually

xavdid

looks awesome! thanks for revising.

xavdid · 2019-10-29T01:43:54Z

packages/cli/src/oclif/commands/scaffold.js

+function createTemplateContext(type, name) {
+  const contextKey = snakeCase(name);
+
+  // where will we create/required the new file?
+  const destMap = {
+    resource: `resources/${contextKey}`,
+    trigger: `triggers/${contextKey}`,
+    search: `searches/${contextKey}`,
+    create: `creates/${contextKey}`
+  };
+
+  return {
+    CAMEL: camelCase(name),
+    KEY: contextKey,
+    NOUN: _.capitalize(name),
+    LOWER_NOUN: name.toLowerCase(),
+    INPUT_FIELDS: '',
+    TYPE: type,
+    TYPE_PLURAL: type === 'search' ? `${type}es` : `${type}s`,
+    // resources need an extra line for tests to "just run"
+    MAYBE_RESOURCE: type === 'resource' ? 'list.' : '',
+    defaultDest: destMap[type]
+  };
+}
+
+function getTemplatePath(type) {
+  return path.join(__dirname, '../../..', `scaffold/${type}.template.js`);
+}


yeah, either way! I think it's reasonable to bundle them all together. Typically we could test them, though these are simple enough we probably won't bother.

xavdid · 2019-10-29T01:45:06Z

packages/cli/src/oclif/commands/scaffold.js

+    if (preventOverwrite && fileExistsSync(destPath)) {
+      const [filename, location] = splitFileFromPath(destPath);
+
+      return this.error(


minor, but you don't need to return here. This'll throw as normal (I think; if you're seeing different behavior, feel free to ignore)

Force of habit to always return something—and also needing to read up on oclif's API 😅

haha yeah. this is a good starting point, plus the TS reference.

I think I'd like it better if it was this.throwError.

If I'm scanning the code and see this.error I don't automatically read that we break out of that block. Whereas return this.error() is crystal clear to me.

xavdid · 2019-10-29T01:45:53Z

packages/cli/src/oclif/commands/scaffold.js

+      // this is very dumb and will definitely break, it inserts lines of code
+      // we should look at jscodeshift or friends to do this instead
+
+      // insert Resource = require() line at top
+      const varName = `${templateContext.CAMEL}${camelCase(type)}`;
+      const importerLine = `const ${varName} = require('./${dest}');`;
+      lines.splice(0, 0, importerLine);
+
+      // insert '[Resource.key]: Resource,' after 'resources:' line
+      const injectAfter = `${typeMap[type]}: {`;
+      const injectorLine = `[${varName}.key]: ${varName},`;
+      const linesDefIndex = lines.findIndex(line => line.endsWith(injectAfter));
+
+      if (linesDefIndex === -1) {
+        this.stopSpinner(false);
+        return this.error(
+          [
+            `\n${colors.bold(
+              `Oops, we could not reliably rewrite your ${entry}.`
+            )} Please add:`,
+            ` * \`${importerLine}\` to the top`,
+            ` * \`${injectAfter} ${injectorLine} },\` in your app definition`
+          ].join('\n')
+        );
+      }
+
+      lines.splice(linesDefIndex + 1, 0, '    ' + injectorLine);


xavdid · 2019-10-29T01:47:18Z

packages/cli/src/utils/string.js

@@ -0,0 +1,13 @@
+function splitFileFromPath(filePath, extension = 'js') {
+  const destParts = filePath.split('/');


🚨 I'd use path.separator here, for cross-platform compatibility. Same below.

xavdid · 2019-10-29T01:48:54Z

packages/cli/src/utils/string.js

+    .concat(`.${extension}`)
+    .join('');
+  const dirPath = destParts.join('/');
+  return [filename, dirPath];


This feels a little backwards to me, I think i'd expect [dirPath, filename] since that's the way it would be written out: src/triggers/foo.js. nbd either way though.

That's a fair point. For some reason I was thinking we were more likely to want the file name vs the dir.

I think that's a fair assumption. honestly, could return { filename, dirPath } and then we can always grab exactly what we want.

I thought about returning an object, but I also like the idea of returning a tuple to make naming the values whatever you want easier—in the case where you call this function twice in the same scope.

const { dirPath: dirA, filename: fileA, } = split(pathA) const { dirPath: dirB, filename: fileB, } = split(pathB) // vs const [dirA, fileA] = split(pathA) const [dirB, fileB] = split(pathB)

And if we wanted just filename

const [,fileName] = split(path)

In the end, it's a minor tradeoff either way—not sure if this function will even get used outside of scaffold.

xavdid · 2019-10-29T03:48:02Z

I can see that. I think I'd be fine to use our own method that calls this.error if we want! No strong feelings.

On Mon, Oct 28, 2019 at 9:35 PM Steve Gardner ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In packages/cli/src/oclif/commands/scaffold.js <#88 (comment)>: > +} + +function getTemplatePath(type) { + return path.join(__dirname, '../../..', `scaffold/${type}.template.js`); +} + +class ScaffoldCommand extends BaseCommand { + async writeTemplateFile(type, templateContext, dest) { + const templatePath = getTemplatePath(type); + const destPath = path.join(process.cwd(), `${dest}.js`); + const preventOverwrite = !this.flags.force; + + if (preventOverwrite && fileExistsSync(destPath)) { + const [filename, location] = splitFileFromPath(destPath); + + return this.error( I think I'd like it better if it was this.throwError. If I'm scanning the code and see this.error I don't automatically read that we break out of that block. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#88?email_source=notifications&email_token=AAJMYP4O2MLCRZYY2VFPKJ3QQ6OOLA5CNFSM4JEPZNFKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCJPXWEI#discussion_r339871768>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJMYP3ULGXLPNLVEIGQKYLQQ6OOLANCNFSM4JEPZNFA> .

-- ~David Brownman Platform Engineer | Zapier <https://zapier.com> c: 303.819.0850 tw: @xavdid

xavdid · 2019-11-12T16:26:24Z

@stevelikesmusic you planning on merging this?

stevelikesmusic · 2019-11-12T19:34:43Z

@xavdid yikes just saw this languishing. Merging now.

Steve Gardner added 2 commits October 24, 2019 18:16

Warn users about overwriting files with scaffold command

d85e46c

Port scaffold to oclif

0a457ba

stevelikesmusic added cli Relates to `zapier-platform-cli` enhancement labels Oct 24, 2019

stevelikesmusic requested review from eliangcs and xavdid October 24, 2019 07:11

stevelikesmusic commented Oct 24, 2019

View reviewed changes

packages/cli/src/oclif/commands/scaffold.js Outdated Show resolved Hide resolved

stevelikesmusic commented Oct 24, 2019

View reviewed changes

packages/cli/src/oclif/commands/scaffold.js Outdated Show resolved Hide resolved

xavdid approved these changes Oct 24, 2019

View reviewed changes

eliangcs approved these changes Oct 25, 2019

View reviewed changes

Steve Gardner added 2 commits October 26, 2019 17:18

Address feedback

542cbec

Add string util. Better separating of file/location from path for err…

279152e

…or message

stevelikesmusic commented Oct 28, 2019

View reviewed changes

xavdid approved these changes Oct 29, 2019

View reviewed changes

Better util implementation. Split out entry updating

e55503d

stevelikesmusic merged commit 6b0fded into master Nov 12, 2019

stevelikesmusic deleted the fix/17-scaffold-warn-overwrite branch November 12, 2019 19:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cli): Scaffold errors if file name exists #88

fix(cli): Scaffold errors if file name exists #88

stevelikesmusic commented Oct 24, 2019

xavdid left a comment

eliangcs left a comment

stevelikesmusic commented Oct 28, 2019

stevelikesmusic Oct 28, 2019

xavdid Oct 29, 2019

stevelikesmusic Oct 28, 2019 •

edited

Loading

stevelikesmusic Oct 28, 2019 •

edited

Loading

xavdid Oct 29, 2019

xavdid left a comment

xavdid Oct 29, 2019

xavdid Oct 29, 2019

stevelikesmusic Oct 29, 2019

xavdid Oct 29, 2019

stevelikesmusic Oct 29, 2019 •

edited

Loading

xavdid Oct 29, 2019

xavdid Oct 29, 2019

stevelikesmusic Oct 29, 2019

xavdid Oct 29, 2019

stevelikesmusic Oct 29, 2019

xavdid Oct 29, 2019

stevelikesmusic Oct 29, 2019

xavdid commented Oct 29, 2019 via email

xavdid commented Nov 12, 2019

stevelikesmusic commented Nov 12, 2019

		@@ -0,0 +1,13 @@
		function splitFileFromPath(filePath, extension = 'js') {

		@@ -0,0 +1,13 @@
		function splitFileFromPath(filePath, extension = 'js') {
		const destParts = filePath.split('/');

fix(cli): Scaffold errors if file name exists #88

fix(cli): Scaffold errors if file name exists #88

Conversation

stevelikesmusic commented Oct 24, 2019

xavdid left a comment

Choose a reason for hiding this comment

eliangcs left a comment

Choose a reason for hiding this comment

stevelikesmusic commented Oct 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevelikesmusic Oct 28, 2019 • edited Loading

Choose a reason for hiding this comment

stevelikesmusic Oct 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xavdid left a 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

stevelikesmusic Oct 29, 2019 • edited Loading

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

xavdid commented Oct 29, 2019 via email

xavdid commented Nov 12, 2019

stevelikesmusic commented Nov 12, 2019

stevelikesmusic Oct 28, 2019 •

edited

Loading

stevelikesmusic Oct 28, 2019 •

edited

Loading

stevelikesmusic Oct 29, 2019 •

edited

Loading