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.

Sign up for GitHub

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

Jump to bottom

fix(inits): .NET semantic fixes for init templates #5154

Merged
merged 5 commits into from
Nov 22, 2019
Merged

fix(inits): .NET semantic fixes for init templates #5154

merged 5 commits into from
Nov 22, 2019

Conversation

NGL321
Copy link
Contributor

@NGL321 NGL321 commented Nov 22, 2019 •
edited
Loading

Several minor semantic .NET template fixes to ensure idiomatic C#. Parameters that default to null do not need to be included by inheritors.

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

@NGL321 NGL321 added the language/dotnet Related to .NET bindings label Nov 22, 2019
@NGL321 NGL321 requested a review from shivlaks November 22, 2019 03:03
@mergify
Copy link
Contributor

mergify bot commented Nov 22, 2019

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

  • PR title type(scope): text
    • type: fix, feat, refactor go into CHANGELOG, chore is hidden
    • scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
    • text: use all lower-case, do not end with a period, do not include issue refs
  • PR Description
    • Rationale: describe rationale of change and approach taken
    • Issues: indicate issues fixed via: fixes #xxx or closes #xxx
    • Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
  • Testing
    • Unit test added. Prefer to add a new test rather than modify existing tests
    • CLI or init templates change? Re-run/add CLI integration tests
  • Documentation
    • README: update module README to describe new features
    • API docs: public APIs must be documented. Copy from official AWS docs when possible
    • Design: for significant features, follow design process

@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Nov 22, 2019
shivlaks
shivlaks reviewed Nov 22, 2019
View reviewed changes
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.

remove .ionide/sysmbolCache.db

shivlaks
shivlaks suggested changes Nov 22, 2019
View reviewed changes
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.

  • remove this binary
  • confirming that you've tested these as well

@NGL321
Copy link
Contributor Author

NGL321 commented Nov 22, 2019 

* remove this binary

* confirming that you've tested these as well

Fixed

shivlaks
shivlaks reviewed Nov 22, 2019
View reviewed changes
...cdk/lib/init-templates/app/csharp/src/%name.PascalCased%/%name.PascalCased%Stack.template.cs Outdated
@@ -4,7 +4,7 @@ namespace %name.PascalCased%
{
public class %name.PascalCased%Stack : Stack
{
public %name.PascalCased%Stack(Construct scope, string id, IStackProps props = null) : base(scope, id, props)
public %name.PascalCased%Stack(Construct scope, string id) : base(scope, id)
Copy link
Contributor

Choose a reason for hiding this comment

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

@RomainMuller pointed out that this would not be a good practice earlier so please hang on until he gets a chance to take a look

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes - the best practice is for Stack subclasses fo accept a StackProps parameter and forward it to the base. I wouldn't encourage users to omit this by encoding it in the template.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • 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

  • 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

  • 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

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@RomainMuller RomainMuller merged commit 04a1b32 into aws:master Nov 22, 2019
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shivlaks shivlaks shivlaks approved these changes

@RomainMuller RomainMuller RomainMuller approved these changes

Assignees

@shivlaks shivlaks

Labels
contribution/core This is a PR that came from AWS. language/dotnet Related to .NET bindings
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@NGL321 @aws-cdk-automation @RomainMuller @shivlaks