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: get the defaultEditor value from the localEnvInfo context variable #9783

Merged

Conversation

flozender
Copy link
Contributor

@flozender flozender commented Feb 16, 2022

Description of changes

Get the default editor value from the context variable when the local-env-info.json file has not been created yet.

Issue #, if available

Closes #8356

Description of how you validated changes

The default editor now shows up on running amplify configure project since the local-env-info.json file now has the defaultEditor key. yarn test also passes.

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)
  • New AWS SDK calls or CloudFormation actions have been added to relevant test and service IAM policies

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Get the defaultEditor value from the context when the local-env-info.json file has not been created
yet

fix aws-amplify#8356
@flozender flozender requested a review from a team as a code owner February 16, 2022 20:40
Copy link
Contributor

@danielleadams danielleadams left a comment

Choose a reason for hiding this comment

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

Hey @flozender, thanks for the contribution. What do you think about adding this to the getEditor method instead as the fallback value in the if/else statement?

@flozender
Copy link
Contributor Author

Ah, yes - that should be better!

@codecov-commenter
Copy link

codecov-commenter commented Feb 22, 2022

Codecov Report

Merging #9783 (2b97805) into master (fcf9100) will increase coverage by 0.15%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9783      +/-   ##
==========================================
+ Coverage   53.10%   53.26%   +0.15%     
==========================================
  Files         830      830              
  Lines       45991    45993       +2     
  Branches     9820     9822       +2     
==========================================
+ Hits        24423    24497      +74     
+ Misses      19556    19487      -69     
+ Partials     2012     2009       -3     
Impacted Files Coverage Δ
...es/amplify-cli/src/init-steps/s0-analyzeProject.ts 69.14% <100.00%> (+29.25%) ⬆️
packages/amplify-cli/src/domain/amplify-toolkit.ts 4.45% <0.00%> (+2.05%) ⬆️
packages/amplify-cli/src/context-extensions.ts 33.75% <0.00%> (+3.75%) ⬆️
...src/extensions/amplify-helpers/editor-selection.ts 96.77% <0.00%> (+6.45%) ⬆️
...ages/amplify-cli/src/init-steps/s1-initFrontend.ts 52.50% <0.00%> (+20.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fcf9100...2b97805. Read the comment docs.

Copy link
Contributor

@danielleadams danielleadams left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

@danielleadams
Copy link
Contributor

I was just looking at this test, and it looks like it may be easy to stub the context out and write a unit test. Do you mind adding one? You can add a s0-analyzeProject.test.ts to the same directory as the test I linked.

@flozender
Copy link
Contributor Author

Sure, I can get that done!

mockContext.exeInfo = {
inputParams: {},
localEnvInfo: {
defaultEditor: 'Visual Studio Code',
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please use a defaultEditor value from this list https://github.com/aws-amplify/amplify-cli/blob/master/packages/amplify-cli/src/extensions/amplify-helpers/editor-selection.ts#L5-L42 . ( please use the value "vscode" instead of the name "Visual Studio Code" in this test)

Copy link
Contributor Author

@flozender flozender Feb 23, 2022

Choose a reason for hiding this comment

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

Sure! I just want to let you know that I modeled my mockContext.exeInfo object by capturing the attribute during amplify init -y and I found the defaultEditor value to be Visual Studio Code, which is why I used the same.

image

If you'd still like, please let me know and I can change it to vscode.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!
it could be that on Windows the executable is named "Visual Studio Code". no need to change then.

@sachscode sachscode merged commit cdb5aec into aws-amplify:master Feb 25, 2022
@github-actions
Copy link

github-actions bot commented Mar 7, 2022

👋 Hi, this pull request was referenced in the v7.6.23 release!

Check out the release notes here https://github.com/aws-amplify/amplify-cli/releases/tag/v7.6.23.

@github-actions github-actions bot added the referenced-in-release Issues referenced in a published release changelog label Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
referenced-in-release Issues referenced in a published release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default editor is shown as undefined after project initialization
6 participants