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

fix(run): add double quotes around script target containing colon #3218

Merged
merged 8 commits into from Jul 6, 2022

Conversation

brees-worth
Copy link
Contributor

@brees-worth brees-worth commented Jul 1, 2022

Description

Adds double-quotes around script target when running with Nx.
Closes #3215

Motivation and Context

Issue #3215
References issue in Nx

How Has This Been Tested?

New unit test added to test suite for command/run. Tested with node 16, npm 8

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (change that has absolutely no effect on users)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Copy link
Collaborator

@JamesHenry JamesHenry left a comment

Thanks a lot for this @brees-worth!

Please kindly fix up the CI and also add some coverage for this in our new (WIP) e2e tests:

e2e/tests/lerna-run/lerna-run.spec.ts

To run the e2e tests you just run:

npm run e2e

You can also forward any arguments onto jest you want, e.g. to only run the lerna-run tests

npm run e2e -- lerna-run.spec.ts

@brees-worth
Copy link
Contributor Author

brees-worth commented Jul 5, 2022

Thanks a lot for this @brees-worth!

Please kindly fix up the CI and also add some coverage for this in our new (WIP) e2e tests:

e2e/tests/lerna-run/lerna-run.spec.ts

To run the e2e tests you just run:

npm run e2e

You can also forward any arguments onto jest you want, e.g. to only run the lerna-run tests

npm run e2e -- lerna-run.spec.ts

All done

@JamesHenry
Copy link
Collaborator

JamesHenry commented Jul 6, 2022

Thanks a lot @brees-worth this is a good case to have in there, but you haven't actually covered the case you are affecting with this change.

Your change is affecting the run-one case, so we need an additional test in there which only invokes the script target for a single project.

Please kindly add coverage for a single target script with and without a :. Please also update the logic to only apply the quotes around the target name if the : is present so that we don't end up with unnecessary quotes in the output for the common case of no : present.

Many thanks again!

@brees-worth
Copy link
Contributor Author

brees-worth commented Jul 6, 2022

Thanks a lot @brees-worth this is a good case to have in there, but you haven't actually covered the case you are affecting with this change.

Your change is affecting the run-one case, so we need an additional test in there which only invokes the script target for a single project.

Please kindly add coverage for a single target script with and without a :. Please also update the logic to only apply the quotes around the target name if the : is present so that we don't end up with unnecessary quotes in the output for the common case of no : present.

Many thanks again!

This is harder than my day job ;)

Fair suggestions, though.
All done.

@JamesHenry JamesHenry changed the title fix(lerna run): add double quotes around script target for Nx fix(run): add double quotes around script target containing colon Jul 6, 2022
@JamesHenry JamesHenry merged commit ead461e into lerna:main Jul 6, 2022
14 checks passed
@JamesHenry
Copy link
Collaborator

JamesHenry commented Jul 6, 2022

Thank you @brees-worth!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants