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

Refactored CMakeServerClient.sendRequest() to improve consistency #949

Merged
merged 3 commits into from
Jan 24, 2020

Conversation

Zingam
Copy link
Contributor

@Zingam Zingam commented Dec 2, 2019

This PR proposes to make CMakeServerClient.sendRequest() private, more readable and its use in line with other calls.

@bobbrow bobbrow added this to the 1.3.0 milestone Dec 4, 2019
@bobbrow
Copy link
Member

bobbrow commented Dec 5, 2019

@Zingam thanks for all of the PR's you've started! It's great to see you getting engaged! I want to apologize in advance that we haven't reviewed them yet, but we do plan to do so in the coming weeks. Our priority was to get 1.2.3 out first, so I've been tagging these with 1.3.0 which is the milestone we plan to include these in.

@Zingam
Copy link
Contributor Author

Zingam commented Dec 5, 2019

@bobbrow Thank you. I hope my PRs are inline with your vision for the extension. I used Visual Studio's CMake integration since its introduction and I reported a ton of issues but sadly it was closed source. I hope to be able to contribute a few features available in Visual Studio 2019 that are still missing in this extension like install and debug #532, which seems will be much more difficult than I have anticipated.

@Zingam
Copy link
Contributor Author

Zingam commented Dec 5, 2019

I also propose to prepend a request or do (e.g. requestCompute) to these methods to better express their purpose in another commit:

  • setGlobalSettings
  • getCMakeCacheContent()
  • getGlobalSettings
  • configure
  • compute
  • codemodel
  • codemodel
  • cmakeInputs

@bobbrow
Copy link
Member

bobbrow commented Dec 5, 2019

I hope my PRs are inline with your vision for the extension.

I would say that pretty much anything that has an open issue that we've responded to is safe. If you are about to start on a large feature/issue, I would recommend you check in with us first in case we have specific goals around it. That will help to avoid any frustration if our ideas around the implementation don't align at first. I hope to draft a roadmap with our priorities soon.

@Zingam Zingam force-pushed the Zingam/Refactor-sendRequest branch from 326db37 to df55d0d Compare January 18, 2020 09:37
@Zingam Zingam changed the title Refactor CMakeServerClient.sendRequest() to improve consistency Refactored CMakeServerClient.sendRequest() to improve consistency Jan 18, 2020
@bobbrow bobbrow merged commit 990c52d into microsoft:develop Jan 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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants