-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Conversation
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.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this 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.
scripts/default.vscode/launch.json
Outdated
// 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. |
There was a problem hiding this comment.
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 :)
scripts/default.vscode/launch.json
Outdated
// Saves some button-pressing latency on attaching | ||
"stopOnEntry": false |
There was a problem hiding this comment.
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)
scripts/init-vscode
Outdated
# 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. |
There was a problem hiding this comment.
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.
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. |
I'm ok putting this in 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 So it's either YOUR magical launch configs or MINE, but not both. |
IIRC it needs to be named I'm also okay with having it in the |
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
@@ -0,0 +1,23 @@ | |||
{ |
There was a problem hiding this comment.
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
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
@@ -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) |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
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). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
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:
preferences, so one set of
.vscode
files would apply to everyone.files would collide.
.vscode
files,we should also start adding
.idea
files and others, and where willit 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