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

Added automatic CMake configure on CMakeLists save #1196

Merged
merged 5 commits into from
Apr 24, 2020

Conversation

Yuri6037
Copy link
Contributor

@Yuri6037 Yuri6037 commented Apr 22, 2020

This change addresses item #1187

This changes visible behavior

The following changes are proposed:

  • Add a file save subscription on extension load
  • Run configure function when the file is named "CMakeLists.txt"

Other Notes/Information

Not sure why but ALT+SHIFT+F to auto reformat changed the whole file. I've checked it and it still works fine on my own projects including a 3D Game Engine.
Also I'm new to TypeScript and Visual Studio Code Extension coding...

@andreeis
Copy link
Contributor

Thank you for investigating a fix for issue #1187. We will have a look. Can you make sure you include in the diffs only the fix and revert the formatting changes?

@Yuri6037
Copy link
Contributor Author

Thank you for investigating a fix for issue #1187. We will have a look. Can you make sure you include in the diffs only the fix and revert the formatting changes?

I just reverted automatic format.

@andreeis andreeis added this to the 1.4.0 milestone Apr 23, 2020
@andreeis
Copy link
Contributor

Your PR failed. Until we fix the way the PR validation is reported, you can find yours at this link: https://travis-ci.org/github/microsoft/vscode-cmake-tools/pull_requests and explore.

For example, the linter suggests these improvements:
ERROR: /home/travis/build/microsoft/vscode-cmake-tools/src/cmake-tools.ts:402:86 - Parentheses are prohibited around the parameter in this single parameter arrow function
ERROR: /home/travis/build/microsoft/vscode-cmake-tools/src/cmake-tools.ts:403:11 - Identifier 'str' is never reassigned; use 'const' instead of 'let'.

When you open the CMake Tools code base folder in VSCode, don't you see anything in the problems tab? You can ensure there are no new reports coming from your code.

Also, the new message you add needs to be localized and it is enough if you do this:
log.debug((localize('cmakelists.save.trigger.reconfigure', "We are saving a CMakeLists.txt file, attempting automatic reconfigure..."));
Where 'cmakelists.save.trigger.reconfigure' is just an arbitrary ID we write now, but soon the localization system will use it to link in all the translations done by the localization team.

@Yuri6037
Copy link
Contributor Author

Your PR failed. Until we fix the way the PR validation is reported, you can find yours at this link: https://travis-ci.org/github/microsoft/vscode-cmake-tools/pull_requests and explore.

For example, the linter suggests these improvements:
ERROR: /home/travis/build/microsoft/vscode-cmake-tools/src/cmake-tools.ts:402:86 - Parentheses are prohibited around the parameter in this single parameter arrow function
ERROR: /home/travis/build/microsoft/vscode-cmake-tools/src/cmake-tools.ts:403:11 - Identifier 'str' is never reassigned; use 'const' instead of 'let'.

When you open the CMake Tools code base folder in VSCode, don't you see anything in the problems tab? You can ensure there are no new reports coming from your code.

Also, the new message you add needs to be localized and it is enough if you do this:
log.debug((localize('cmakelists.save.trigger.reconfigure', "We are saving a CMakeLists.txt file, attempting automatic reconfigure..."));
Where 'cmakelists.save.trigger.reconfigure' is just an arbitrary ID we write now, but soon the localization system will use it to link in all the translations done by the localization team.

That's strange I can run the code perfectly fine under VS Code. I don't know how you got those errors... I will try to fix from what you told me.

@Yuri6037
Copy link
Contributor Author

Also thank you for the Travis Link I was searching for it but couldn't find it. I will base myself on the Travis for checking build...

@Yuri6037
Copy link
Contributor Author

Yuri6037 commented Apr 23, 2020

What should I do about the fakecontext ? It seam that Unit tests are not designed to support accessing subscriptions on the extension context.

In addition it seems that a lot of tests fails in parts of the code that I did not even change. Can you have a look at those other failing tests? I will try to allow the test tool to accept subscriptions.

EDIT2: It seems that fixing the first test fixed subsequent tests.

EDIT3: All fixed unit tests are now running.

@andreeis
Copy link
Contributor

What should I do about the fakecontext ? It seam that Unit tests are not designed to support accessing subscriptions on the extension context.

In addition it seems that a lot of tests fails in parts of the code that I did not even change. Can you have a look at those other failing tests? I will try to allow the test tool to accept subscriptions.

EDIT2: It seems that fixing the first test fixed subsequent tests.

EDIT3: All fixed unit tests are now running.

I don't know very well, investigating. I wonder if the failing tests are a sign that maybe another place would be better for the subscription, which would not cause any test failure. Otherwise, the approach doesn't look too bad to me.

@andreeis
Copy link
Contributor

Looks good. I am going to approve. Ideally, this change could have been accompanied by a new test but it's ok now. We need to fix something else very similar which will require some new tests and we will cover this scenario among them.
Thank you very much for your contribution.

@andreeis andreeis merged commit bae775f into microsoft:develop Apr 24, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Jan 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants