fix: update cli Generator forceInstall parameter to install

[sebastian-palma]

This is a very small change. As the cli instantiates a new Generator object with the given parameters, it uses forceInstall as the option to install the given template or not. But in the generator file the constructor definition it's done by deconstructing the third argument and the argument for installing the template is forceInstall not install.

So, no matter if you specify you want to install the template, Generator.install is always false.

I could've modified all the matches to forceInstall to make it clear you want to force the installation of the template, but I don't know if that's the case - as forceWrite is indeed forcing to write the changes. I better modified a single line to make the forceInstall option to be just install.

I noticed this "failed" silently as the cli file isn't tested, which makes me ask, would it be better to add full coverage to that file in order to merge this? Or, would it be easier to add a guard clause in the Generator constructor body to return if it receives an unexpected parameter (I think it'd be easier)?

By the way, thanks for this amazing tool.

[derberg]

@sebastian-palma great catch! both solutions proposed by you are good but better is the one that validates incoming parameters, it's adding protection in case we make errors in cli.js and nice validation for those using the generator as a library. Would you be willing to add it to this PR or separate one?

[sebastian-palma]

Cool @derberg, I can keep working in this PR.

[sebastian-palma]

Hey @derberg, I just committed some changes. Would you mind helping me here to see if everything is okay?

[derberg]

Oh man you're fast! Thanks a lot for your work.
I added few comments, one is that you missed one important option, and others are for discussion on the code structure and location

[derberg]

damn I lost one comment, let we write again :(

what do you think about splitting responsibility of validateOptions so it is only responsible to getInvalidOptions and the building of the error string stays in generator? then you can avoid things like arguments.length === 3

something like:

const invalidOptions = getInvalidOptions(arguments[arguments.length - 1] || [])
if (invalidOptions.length) throw new Error(`These options are not supported by the generator: ${invalidOptions.join(', ')}`);

I'm super curious what do you think about it

[sebastian-palma]

Nice one, I thought the same.

As per the arguments.length === 3, it was there to avoid throwing a new error in case a third argument isn't given, as Object.keys() is receiving the last argument always and when a string is given it returns an array of strings corresponding to its length.

I'm gonna figure out how to return just the invalid options, which would make it easier 👌.

[sebastian-palma]

Hey @derberg, just moved some things. Looking forward to another round of review. I'm wondering if it's okay with the guard clauses and your style guide.

[derberg]

we kind of still have it here an error message hidden in the variable which is not the best as while reading this and next line I want to have a clear answer what error is thrown when

best experience would be to have

const invalidOptions = getInvalidOptions(GENERATOR_OPTIONS, arguments[arguments.length - 1])
if (getInvalidOptions.length) throw new Error(`These options are not supported by the generator: ${invalidOptions.join(', ')}`);

why:

• GENERATOR_OPTIONS belongs to a file where the generator constructor is, for maintenance reasons
• error message reason mentioned at the beginning

then just merge invalidOptions with getInvalidOptions function that should not be marked as private.

---

Despite my comment above I still have a big concern about how we get GENERATOR_OPTIONS up to date, as if I forget to put new options there, no tests will fail. You noticed that in the initial implementation you forgot about debug

[derberg]

https://github.com/dsheiko/bycontract looks interesting but I haven't tried it, but it talks about jsdoc that we at least always keep up to date

[sebastian-palma]

Remove invalidOptions, really I don't know why both were marked as private, maybe too much time looking to the same piece of code 🙂

Despite my comment above I still have a big concern about how we get GENERATOR_OPTIONS up to date, as if I forget to put new options there, no tests will fail. You noticed that in the initial implementation you forgot about debug

That's a good question. It'd be easy if JS provides keyword arguments, so we use them to reference the mandatory ones one and everything else can be passed as an optional object, but it doesn't.

Or we could define the generator options within the class, and dynamically build the constructor by iterating through its element, so it acts as the single source of truth. E.g:

class Generator {
  constructor(templateName, targetDir, options) {
    const GENERATOR_OPTIONS = ['debug', 'disabledHooks', 'entrypoint', 'forceWrite', 'install', 'noOverwriteGlobs', 'output', 'templateParams'];
    GENERATOR_OPTIONS.forEach(option => {
      this[option] = options[option];
    });
  }
}

let gen = new Generator('foo', 'bar', { debug: 'a', disabledHooks: 'b', entrypoint: 'c' });
gen.debug            // 'a'
gen.disabledHooks    // 'b'
gen.entrypoint       // 'c'
gen.forceWrite       // undefined
gen.install          // undefined
gen.noOverwriteGlobs // undefined
gen.output           // undefined
gen.templateParams   // undefined

[derberg]

looks nice but then you lose what is nice currently, that options are destructured in params and we can use defaults there. I'm afraid now adding to your suggesting defaults, would complicate stuff 🤔
did you try the library I mentioned? sounds like the perfect choice

[sebastian-palma]

nono I haven't tried it still, I wanted to know if we have the chance to do it without libraries.

Would you like to try the library and incorporate it to this PR or do a separate one for that?

[derberg]

nah, let's leave it, I was just curious, to use this type of decorators we would probably have to use babel or something anyway.

Just please move GENERATOR_OPTIONS to generator.js so we maintain the list where the constructor is, and just extend getInvalidOptions with an additional argument.

I think that whatever option we choose here to have "future errors secure implementation" will just add complexity that I'm not sure is worth it. Let's ignore my initial concern and react properly in the future where we see it is really a problem

[derberg]

@sebastian-palma we want to release 1.0.0-rc1 and it would be great to have this fix for the CLI in there. Do you agree with my comment and still want to work on it, or did you loose your patience because of me being picky 😅

[sebastian-palma]

Haha not at all @derberg, very nice working with you guys ;)

I'm gonna move GENERATOR_OPTIONS, would that be fine for making the release? We can start playing with the library right after the merge.

[derberg]

yes, definitely, forget about this library, the annotation I like in it works only if we would plugin in babel, and I do not want such a tiny thing to add babel to the project. The other options the library has is adding as much code as we would write it on our own.

so as I wrote, let's just see if my concern is real in real life

[sebastian-palma]

Hey @derberg, just committed and squashed the changes. Sorry for the delay 😁

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication

[derberg]

lgtm
@sebastian-palma thanks a lot for this contribution and thanks so much for your patience here

[asyncapi-bot]

🎉 This PR is included in version 0.53.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[derberg]

@all-contributors please add @sebastian-palma for code

[allcontributors]

@derberg

I've put up a pull request to add @sebastian-palma! 🎉