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 custom commands in dev mode #4650

Merged
merged 2 commits into from
Jun 20, 2023
Merged

fix custom commands in dev mode #4650

merged 2 commits into from
Jun 20, 2023

Conversation

shumailxyz
Copy link
Contributor

@shumailxyz shumailxyz commented Jun 16, 2023

What this PR does / why we need it:

  • Fixes custom commands not working on first run in dev mode.
  • Also uses command artifact for version command so it is supported in dev mode and shows up in help.

Which issue(s) this PR fixes:

Fixes #4647

Special notes for your reviewer:
The second issue with custom gardenCommands reported in #4647 will be address separately. Tracking that with #4668

@vvagaytsev
Copy link
Collaborator

vvagaytsev commented Jun 16, 2023

Just a nit on the commit message.

fix: make a copy of projectRoots instead of using reference

This will appear as "make a copy of projectRoots instead of using reference" in the list of bug fixes in the release notes. This sounds more like implementation details. We should consider using a brief summary addressing the issue as a commit header. And the details can go to the message body.

Something like this:

fix: allow (or enable/fix missing/whatever is the best here :)) custom commands in the dev console

Make a defensive copy of `projectRoots` instead of using a reference when memorizing old values before the reload.

Copy link
Collaborator

@edvald edvald left a comment

Choose a reason for hiding this comment

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

One comment, rest looks solid. Good catch on the projectRoots thing, that's a subtle one.

core/src/commands/version.ts Show resolved Hide resolved
core/src/cli/cli.ts Outdated Show resolved Hide resolved
@shumailxyz shumailxyz force-pushed the fix/dev-mode-cmds branch 2 times, most recently from a76fd6e to 9117243 Compare June 20, 2023 12:58
@shumailxyz shumailxyz marked this pull request as ready for review June 20, 2023 13:27
@shumailxyz shumailxyz requested review from vvagaytsev and a team June 20, 2023 13:27
@vvagaytsev
Copy link
Collaborator

vvagaytsev commented Jun 20, 2023

A minor nit. It looks like commits bd0e6ce and 9117243 can be squashed.

I'd also consider it as an improvement instead of a bug fix :)

replaces the old version artifacts with a version command artifact that is supported
in dev mode and shown in help
Make a defensive copy of `projectRoots` instead of using a reference when
memorizing old values before the reload.
@shumailxyz
Copy link
Contributor Author

@vvagaytsev Thanks. Updated it to reflect improvement and combined both commits into one. Had to force push though.

Copy link
Collaborator

@vvagaytsev vvagaytsev left a comment

Choose a reason for hiding this comment

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

Great job, thank you! 🚀

@shumailxyz
Copy link
Contributor Author

@vvagaytsev Many thanks for pairing on this 🙌

@shumailxyz shumailxyz merged commit 3dda63e into main Jun 20, 2023
@shumailxyz shumailxyz deleted the fix/dev-mode-cmds branch June 20, 2023 14:22
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.

custom commands broken in dev console
3 participants