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

chore: add a base VSCode config #8184

Merged
merged 6 commits into from
Jun 1, 2020
Merged

chore: add a base VSCode config #8184

merged 6 commits into from
Jun 1, 2020

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented May 25, 2020

VSCode configs for our repo are tricky enough that people would benefit
from having them checked into the repo. Some people are strongly opposed
to having them checked in at the default location though, for what I
assume are the following reasons:

  • There's no good way to have user-specific, workspace-specific
    preferences, so one set of .vscode files would apply to everyone.
  • If you already had workspace-specific VSCode preferences, the new
    files would collide.
  • Not everyone uses VSCode, so if we start adding .vscode files,
    we should also start adding .idea files and others, and where will
    it end, and who's going to keep them consistent?

As a compromise, adding a script which will copy a base VSCode config
into place. You can choose the run the script if you want it, and you
can choose not to run it if you don't. Everybody happy, right?

If necessary, we'll be able to extend this in the future with custom
per-user configs, but for now let's start with something simple.


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

VSCode configs for our repo are tricky enough that people would benefit
from having them checked into the repo. Some people are strongly opposed
to having them checked in at the default location though, for what I
assume are the following reasons:

- There's no good way to have user-specific, workspace-specific
  preferences, so one set of `.vscode` files would apply to everyone.
- If you already had workspace-specific VSCode preferences, the new
  files would collide.
- Not everyone uses VSCode, so if we start adding `.vscode` files,
  we should also start adding `.idea` files and others, and where will
  it end?

As a compromise, adding a script which will copy a base VSCode config
into place.  You can choose the run the script if you want it, and you
can choose not to run it if you don't. Everybody happy, right?

If necessary, we'll be able to extend this in the future with custom
per-user configs, but for now let's start with something simple.
@rix0rrr rix0rrr requested a review from a team May 25, 2020 13:17
@rix0rrr rix0rrr self-assigned this May 25, 2020
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label May 25, 2020

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: cd20fbf
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 11b75e6
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Copy link
Contributor

@shivlaks shivlaks left a comment

Choose a reason for hiding this comment

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

seems like a reasonable compromise based on the commit body.
nice to add it into GitPod experience as well.

It might be useful to add a couple lines into CONTRIBUTING.md for other contributors to discover and try.

Comment on lines 14 to 15
// If we don't do this, every step-into into an async function call will go into
// NodeJS internals which are hard to step out of.
Copy link
Contributor

Choose a reason for hiding this comment

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

this could have saved me some pain a few months ago :)

Comment on lines 19 to 20
// Saves some button-pressing latency on attaching
"stopOnEntry": false
Copy link
Contributor

Choose a reason for hiding this comment

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

is it really button-pressing latency?
doesn't setting this to true cause it to break when the app launches (even if there are no breakpoints)

Comment on lines 2 to 4
# For people that want a base VSCode setup (with useful launchers etc)
# But without checking in a .vscode directory by default: there are
# people who definitely do not want it checked in.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can probably be simplified to describe the behavior. I had to do a double take to understand what I'm getting by running this.

@eladb
Copy link
Contributor

eladb commented May 26, 2020

What the objection for putting this at the root so it will automatically be identified?

@rix0rrr
Copy link
Contributor Author

rix0rrr commented May 26, 2020

What the objection for putting this at the root so it will automatically be identified?

Paging @RomainMuller who I think has the strongest opinions on this.

Also, I literally described the objections in the PR body.

@rix0rrr rix0rrr requested a review from a team May 26, 2020 07:55
@rix0rrr rix0rrr requested a review from RomainMuller May 26, 2020 12:17
@nija-at
Copy link
Contributor

nija-at commented May 26, 2020

I'm ok putting this in .vscode/ folder. That's much better than what this PR is trying to do.

If we choose a not so common name to the launch config, it's unlikely to collide. That solves it for this PR at least.

@rix0rrr
Copy link
Contributor Author

rix0rrr commented May 26, 2020

If we choose a not so common name to the launch config, it's unlikely to collide. That solves it for this PR at least.

Well it's going to collide by the mere fact that you could already have a launch.json file in your directory, and they can't both exist.

So it's either YOUR magical launch configs or MINE, but not both.

@shivlaks
Copy link
Contributor

I'm ok putting this in .vscode/ folder. That's much better than what this PR is trying to do.

If we choose a not so common name to the launch config, it's unlikely to collide. That solves it for this PR at least.

IIRC it needs to be named launch.json

I'm also okay with having it in the .vscode/ folder as the objections mentioned in the commit body don't resonate with me.

@rix0rrr
Copy link
Contributor Author

rix0rrr commented May 27, 2020

This seems like we have 3 people in favor of just adding default config (4 if you count me, which I am!), and 1 known objector who has not commented.

Changing the PR then!

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 71a644d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@rix0rrr rix0rrr requested a review from shivlaks May 27, 2020 11:08
@@ -0,0 +1,23 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Update the contribution guide accordingly with the steps needed to configure a CDK app to successfully connect to a debugger? I don't think it's trivial for someone new-ish to the code base.

https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md#connecting-the-vs-code-debugger

@rix0rrr rix0rrr requested a review from nija-at May 29, 2020 13:34
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 06d3519
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 737cb8d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@nija-at nija-at added the pr/do-not-merge This PR should not be merged at this time. label May 29, 2020
@@ -43,6 +43,7 @@ and let us know if it's not up-to-date (even better, submit a PR with your corr
- [Troubleshooting](#troubleshooting)
- [Debugging](#debugging)
- [Connecting the VS Code Debugger](#connecting-the-vs-code-debugger)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe change this to 'run a cdk app in the debugger' to keep it all consistent.

@@ -910,6 +911,24 @@ To debug your CDK application along with the CDK repository,
6. The debug view, should now have a launch configuration called 'Debug hello-cdk' and launching that will start the debugger.
7. Any time you modify the CDK app or any of the CDK modules, they need to be re-built and depending on the change the `link-all.sh` script from step#2, may need to be re-run. Only then, would VS code recognize the change and potentially the breakpoint.

### Run a CDK unit test in the debugger
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to rewrite the above section to use the same launch configuration and drop the step that asks the user to configure their own launch section?

Would help simplify the contribution guide, but I'll leave it to you to decide.

@rix0rrr rix0rrr removed the pr/do-not-merge This PR should not be merged at this time. label Jun 1, 2020
@mergify
Copy link
Contributor

mergify bot commented Jun 1, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 351df5a
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Jun 1, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 7d60681 into master Jun 1, 2020
@mergify mergify bot deleted the huijbers/vscode-settings branch June 1, 2020 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants