Skip to content

feat(cli): ask to override files#475

Merged
davidverdu merged 23 commits intoboostercloud:contribfrom
fecony:feat/overriding_files
Feb 18, 2021
Merged

feat(cli): ask to override files#475
davidverdu merged 23 commits intoboostercloud:contribfrom
fecony:feat/overriding_files

Conversation

@fecony
Copy link
Contributor

@fecony fecony commented Oct 24, 2020

Description

Check if project folder or resource already exist before generating new file. It will ask user if he wants to override existing file and continue or stop generating. If user agrees to override it file/project folder is removed. If declines then it will throw error for user to use different resource name. (See Images below)

Closes #305

Changes

  • Checks if project or file already exist.
  • export projectDir()
  • Adds checkProjectAlreadyExists() to check if project already exists in current directory.
  • Adds checkResourceExists() to check if resource already exists.
  • Unit tests for checkProjectAlreadyExists() and checkResourceExists() methods
  • Adds confirmPrompt() which is used by checkProjectAlreadyExists() and checkResourceExists() to reuse same code
  • Adds filePath function which was used in single method, to get path where resource should be generated. Is used for generation and to check if resource exists
  • Adds FileDir type for filePath method to pick only required params 'placementDir' | 'name' | 'extension'
  • Adds getResourceType() to get resource name from path.

Checks

  • Project Builds
  • Project passes tests and checks
  • Updated documentation accordingly

Additional information

Preview of CLI new messages

Screenshot 2020-10-21 at 23 12 36

Screenshot 2020-10-21 at 23 12 40

@NickSeagull NickSeagull changed the title feat(cli): ask to override files [WIP] feat(cli): ask to override files Oct 24, 2020
@javiertoledo
Copy link
Member

FYI, #476 changes significantly how CLI integration tests work, so I'd avoid changing/implementing integration tests in this PR until #476 is merged. If we review/approve this faster, we can merge this PR and implement the tests in a separate issue or keep this PR open until the other is merged.

@fecony fecony marked this pull request as draft October 25, 2020 14:14
@javiertoledo
Copy link
Member

javiertoledo commented Oct 25, 2020

Hey @fecony, just tool a deeper look at your code and I have some thoughts/ideas/suggestions:

  1. Checking the existence of the files in the commands could end up being a bit fragile for future changes because someone could forget about it when creating new commands or delete the check accidentally, and I like the idea of making these checks the default for code generators. I think I'd do it directly in the generate function in packages/cli/src/services/generator.ts.

  2. There's a Prompter class in user-prompt.ts with a couple of methods using inquirer. It would be nice to put the confirmPrompt function in there to keep consistency.

  3. I liked the idea of having this project-checker service to encapsulate this functionality, and as it's designed in this nice functional fashion, I think you should be able to use it from the generate function or any other place where you decide to put the checks.

What do you think? Maybe there's a better place to put the checks, but I'd definitely avoid doing it explicitly in the commands.

@fecony
Copy link
Contributor Author

fecony commented Oct 28, 2020

Hey, @javiertoledo.

  1. I have moved promt to Prompter class.
  2. Moved resource existence check to generate method

Now it will check if resource exists and print info message about it before printing prompt or generation text.
Screenshot 2020-10-28 at 21 36 45

Copy link
Member

@javiertoledo javiertoledo left a comment

Choose a reason for hiding this comment

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

Good work! The code is looking great, the only thing I'm missing is unit tests checking that the prompt is printed when a file exists and what happens when the user chooses to override or cancel the action. Once we merge #476, we could come back to this and implement a couple of integration tests too. Thanks a lot!

@fecony
Copy link
Contributor Author

fecony commented Oct 31, 2020

@javiertoledo Yep, #476 with it createSandboxProject is something I would like to use in unit tests too, I tried to write unit tests for this PR, but faced problems with creating / clearing test project files. Let me know when I can continue working on this PR to implement integration tests!

edit: Need to update docs too. There was something about files being removed.

@javiertoledo
Copy link
Member

@fecony #476 was merged, so you can rebase and use the createSandboxProject helper function now :-)

@fecony
Copy link
Contributor Author

fecony commented Nov 13, 2020

@javiertoledo Hey! I started to work with unit tests and I am confused with createSandboxProject method...

The problem is, it copies src folder of packages/cli (the origin folder where test is running) folder and not integrations tests src/ folder that has test files(?)

I am not sure if it was planned to use createSandboxProject method only in integration tests and that means I am doing some 💩 with it in unit tests. But I don't know 😄

Let me know if I am wrong and should not use it in unit tests. Also if it is the case maybe you can tell me what exactly I should test in unit tests? 🤔

@javiertoledo
Copy link
Member

@fecony yes, the createSandboxProject package is designed to run in the integration tests only. This is because in unit tests we're mocking the interfaces and focusing on checking that all methods are calling to the right APIs with the right parameters, so no real files should be checked or created. This makes unit tests faster and more reliable (this is good because we run them often during dev). Integration tests do the real stuff; they're slower and run primarily in the CI/CD process. In the integration tests package, you can find a couple of cli integration tests. Those tests are creating this sandbox folder where they can safely mess with actual files. Once the test is done, we remove the sandbox folder.

Copy link

@NickSeagull NickSeagull left a comment

Choose a reason for hiding this comment

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

Lovely, thanks for this! The only thing I'm missing is what @javiertoledo pointed :D

Copy link
Contributor

@alvaroloes alvaroloes left a comment

Choose a reason for hiding this comment

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

@fecony Love the PR! Thanks a lot for this. Once you have the unit test in place (those Javi mentioned) and checked that the documentation is updated, this PR is good to go. Pre-approving!

Base automatically changed from master to main February 1, 2021 17:13
@javiertoledo javiertoledo added this to the v0.11.0 - Next minor version milestone Feb 3, 2021
@javiertoledo javiertoledo added package:cli Affects the CLI package feature-request New feature or request size: M Tasks where you have to change some files. labels Feb 3, 2021
@davidverdu
Copy link
Collaborator

Hi @fecony great work!! I think we have almost ready to merge. We want to move this PR to improve the framework. We also need to rebase with main before merging. Do you need help? Please join our Discord community to solve doubts. Thank you!

@fecony
Copy link
Contributor Author

fecony commented Feb 4, 2021

@davidverdu, @javiertoledo, uff, I tried to write tests last time in november...
After that I switched to my job. and quite forgot about booster.. I am reading about new things happening in it. But I don't know much what has changed.

I may try to write tests on this weekend. If you didn't decide to take take care of this PR 😄

@fecony fecony force-pushed the feat/overriding_files branch from a56b647 to cbf508f Compare February 5, 2021 14:23
@fecony
Copy link
Contributor Author

fecony commented Feb 13, 2021

@davidverdu, @javiertoledo rebased & added unit tests!
I hope now it can be merged. If there is anything this PR is missing, let me know 😄

@davidverdu
Copy link
Collaborator

@davidverdu, @javiertoledo rebased & added unit tests!
I hope now it can be merged. If there is anything this PR is missing, let me know 😄

Hi @fecony there is a unit test which is failing. Could you review it? If you need help let us know. To run the unit tests just type lerna run test --stream

@fecony
Copy link
Contributor Author

fecony commented Feb 17, 2021

Hey, @davidverdu fixed unit tests 📦

@davidverdu
Copy link
Collaborator

Hey, @davidverdu fixed unit tests 📦

Thank you so much @fecony . However, the integration tests, which are run with lerna run integration --stream are broken because 2 tests are trying to create files that already exist and get blocked with the prompt. To fix this is easy:

In packages/framework-integration-tests/integration/cli/cli.event.integration.ts, go to the case without fields => should create new event and add the line removeFiles([FILE_CART_CHANGED_EVENT]) before executing the command.

A similar change is needed in the file packages/framework-integration-tests/integration/cli/cli.readmodel.integration.ts. Remember to add the dependency at the top of each file.

Another thing you need to do is to rebase again with main, since we have added new lines of code.

Integration tests must work properly before accepting the PR. Thank you, you are almost ready for merge!

Copy link
Collaborator

@davidverdu davidverdu left a comment

Choose a reason for hiding this comment

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

Integration tests need to be fixed. I left a commend with instructions to solve the problem

@boostercloud boostercloud locked as resolved and limited conversation to collaborators Feb 17, 2021
@boostercloud boostercloud unlocked this conversation Feb 17, 2021
@davidverdu davidverdu changed the base branch from main to contrib February 18, 2021 11:14
Copy link
Collaborator

@davidverdu davidverdu left a comment

Choose a reason for hiding this comment

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

Integration tests have been fixed and the pass locally. Let's merge to contrib branch first, in order to run integration tests in GitHub, with the AWS credentials to run the tests

@boostercloud boostercloud locked as resolved and limited conversation to collaborators Feb 18, 2021
@boostercloud boostercloud unlocked this conversation Feb 18, 2021
@davidverdu davidverdu merged commit fd906ff into boostercloud:contrib Feb 18, 2021
@fecony fecony changed the title [WIP] feat(cli): ask to override files feat(cli): ask to override files Feb 19, 2021
@fecony fecony deleted the feat/overriding_files branch February 19, 2021 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature-request New feature or request package:cli Affects the CLI package size: M Tasks where you have to change some files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ask the user if they want to override existing files while using generators

6 participants