Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(remix-dev/cli): simplify validateNewProjectPath in create command #2695

Merged
merged 3 commits into from
Apr 9, 2022

Conversation

XiNiHa
Copy link
Contributor

@XiNiHa XiNiHa commented Apr 8, 2022

Removes a dead branch found inside the CLI code for validating project directories.

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Apr 8, 2022

Hi @XiNiHa,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run.

Thanks!

- The Remix team

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Apr 8, 2022

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@machour machour changed the title chore(cli): remove dead branch chore(cli): remove dead code branch Apr 8, 2022
@MichaelDeBoey MichaelDeBoey changed the title chore(cli): remove dead code branch chore(remix-dev/cli): remove dead code branch Apr 8, 2022
@MichaelDeBoey
Copy link
Member

@XiNiHa Code changes go against dev branch. Please rebase your branch against latest dev & make sure to fix all tests.

@MichaelDeBoey MichaelDeBoey added the needs-response We need a response from the original author about this issue/PR label Apr 8, 2022
@XiNiHa XiNiHa changed the base branch from main to dev April 8, 2022 14:28
@XiNiHa
Copy link
Contributor Author

XiNiHa commented Apr 8, 2022

Not sure if I did it right or not, but tried to do what you've requested

@github-actions github-actions bot removed the needs-response We need a response from the original author about this issue/PR label Apr 8, 2022
Copy link
Member

@MichaelDeBoey MichaelDeBoey left a comment

Choose a reason for hiding this comment

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

@XiNiHa I just looked into your PR and you're indeed removing dead code.
I however think we should only move the return into the contents.length check, as that's what would be intended I think.

@XiNiHa
Copy link
Contributor Author

XiNiHa commented Apr 8, 2022

@MichaelDeBoey The diff is a bit confusing, but the code does the same thing (I just moved the contents part into the if conditions)

@MichaelDeBoey
Copy link
Member

@XiNiHa You also removed one of the possible errors.
I think you should revert that.

@XiNiHa
Copy link
Contributor Author

XiNiHa commented Apr 8, 2022

@MichaelDeBoey It was impossible to hit that error. The condition ((await fse.pathExists(projectDir)) && (await fse.stat(projectDir)).isDirectory()) was same for two ifs, and the first if throws when contents.length > 0, and returned otherwise. Therefore it had no chance to hit the latter error, since the second if will never be visited. I believe that was intended to return on !(contents.length > 0) since it's safe to override the contents inside the directory in that case. I'm very sure that the code is functioning exactly same.

@MichaelDeBoey
Copy link
Member

@XiNiHa The second one was indeed never hit, but I'm quite certain that was never @kentcdodds' intention in #2655.

Since that wasn't his intention, the return should move 1 line higher to get the intended code.

@XiNiHa
Copy link
Contributor Author

XiNiHa commented Apr 8, 2022

@MichaelDeBoey I guess this is what you want, but I'm still not sure if it's a good idea to error when users try to create a project on an empty directory.

@MichaelDeBoey MichaelDeBoey changed the title chore(remix-dev/cli): remove dead code branch chore(remix-dev/cli): simplify validateNewProjectPath in create command Apr 8, 2022
@MichaelDeBoey
Copy link
Member

Let's ask @kentcdodds what his intentions were to be certain of it

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Thank you!

@kentcdodds kentcdodds merged commit ac622ca into remix-run:dev Apr 9, 2022
@XiNiHa XiNiHa deleted the remove-dead-branch branch April 9, 2022 02:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants