Skip to content
This repository has been archived by the owner on Dec 16, 2022. It is now read-only.

refactor(index): rewrite answer parsing #541

Merged
merged 20 commits into from
Dec 2, 2021
Merged

Conversation

Haroenv
Copy link
Contributor

@Haroenv Haroenv commented Nov 25, 2021

There were a couple things that didn't work as expected

  • isQuestionAsked actually implemented shouldQuestionBeAsked (opposite to the name)
  • getOptionsFromArguments isn't needed, Commander already exposes arguments
  • splitting array manually can be done in Commander
  • confusing "when" conditional for array values
  • validation of appName can be done inline so it doesn't error
  • move post processing of answers into its own helper
  • asks questions that should still be asked, unless --config or --no-interactive is provided

BREAKING CHANGE: it now asks questions if some of the parameters are sent via arguments. Before this, giving an argument would cause it not to ask any questions anymore, even if they still would be useful. You can avoid this behaviour by passing --config or --no-interactive

There were a couple things that didn't work as expected

- isQuestionAsked actually implemented shouldQuestionBeAsked (opposite to the name)
- getOptionsFromArguments isn't needed, Commander already exposes arguments
- splitting array manually can be done in Commander
- confusing "when" conditional for array values
- validation of appName can be done inline so it doesn't error
@Haroenv Haroenv marked this pull request as ready for review November 25, 2021 18:28
@Haroenv Haroenv requested review from a team, dhayab and tkrugg and removed request for a team November 25, 2021 18:35
Copy link
Member

@francoischalifour francoischalifour left a comment

Choose a reason for hiding this comment

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

The code change looks good overrall.

However, why do we now ask questions when the necessary options were passed to the CLI?

This original behavior was made as a convenience. The release script that generates the demos will also probably fail now.

src/cli/__tests__/getConfiguration.test.js Show resolved Hide resolved
src/cli/index.js Outdated Show resolved Hide resolved
e2e/installs.test.js Outdated Show resolved Hide resolved
e2e/installs.test.js Outdated Show resolved Hide resolved
Copy link
Member

@francoischalifour francoischalifour left a comment

Choose a reason for hiding this comment

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

Running the CLI without name is broken:
image

src/cli/index.js Outdated Show resolved Hide resolved
Copy link
Member

@francoischalifour francoischalifour left a comment

Choose a reason for hiding this comment

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

Nice change!

message: 'Project directory',
const isValid = Boolean(input);
if (!isValid) {
console.log('template is required');
Copy link
Member

Choose a reason for hiding this comment

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

This console.log shouldn't be required, our script errors by itself like with other options, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it would error in

throw new Error('The template is required in the config.');
, but in
process.exit(1);
the validation functions are used before getConfiguration is called, which is why there's two "paths" to requiring template

@Haroenv Haroenv merged commit efd2799 into master Dec 2, 2021
@Haroenv Haroenv deleted the refactor/correct-index branch December 2, 2021 14:01
aymeric-giraudet pushed a commit to algolia/instantsearch that referenced this pull request Dec 14, 2022
…-app#541)

* refactor(index): rewrite answer parsing

There were a couple things that didn't work as expected

- isQuestionAsked actually implemented shouldQuestionBeAsked (opposite to the name)
- getOptionsFromArguments isn't needed, Commander already exposes arguments
- splitting array manually can be done in Commander
- confusing "when" conditional for array values
- validation of appName can be done inline so it doesn't error

BREAKING CHANGE: the program now asks questions if some of the parameters are sent via arguments. Before this, giving an argument would cause it not to ask any questions anymore, even if they still would be useful. You can avoid this behaviour by passing --config or --no-interactive

* postprocess answers

* e2e installs should not have any asked questions

* make e2e build test pass

* more info

* update to newer commander to fix the "name" issue

* refactor: validate appPath like appName

* error for empty string path

* clean lockfile

* refactor initial questions

* silently check for existing answers on appName

* don't ask initial questions if config is set

* introduce --no-interactive, mostly for test

* write tests

* fix argument

* undo change

* make .default apply always

* refactor templates to be part of initialQuestions

* cover initialQuestions in tests

* fix error
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants