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

naming conventions: 'add' methods #1741

Closed
rix0rrr opened this issue Feb 12, 2019 · 6 comments
Closed

naming conventions: 'add' methods #1741

rix0rrr opened this issue Feb 12, 2019 · 6 comments
Assignees
Labels
@aws-cdk/core Related to core CDK functionality feature-request A feature should be added or improved. package/awscl Cross-cutting issues related to the AWS Construct Library

Comments

@rix0rrr
Copy link
Contributor

rix0rrr commented Feb 12, 2019

We have methods named foo.addBar() which do two different things:

  • One, to take an existing bar object and relate it to the foo somehow.
  • Second, to construct a new bar object (typically using some properties from foo to make the API nicer), relate it to foo, and then return it.

Examples:

// Adds an EXISTING AutoScalingGroup
ecsCluster.addAutoScalingGroup(asg);

// Creates a NEW AutoScalingGroup. 
// Advantage: don't need to specify what VPC the ASG needs to
// live in, it will be the default one for the cluster.
ecsCluster.addCapacity('Machines', {
  desiredCapacity: 5
});

// Creates a NEW listener.
// Advantage: don't need to relate the Listener to the Load Balancer,
// will automatically be the correct one.
loadBalancer.addListener('Listener', {
  port: 80
});

// Relates the EXISTING alarm to the DeploymentGroup
deploymentGroup.addAlarm(alarm);

I propose that we rename all methods that create new constructs to newBar() and their signature must always be foo.newBar(id, props)

The type should probably be called Props instead of Options (see #1740)

@eladb
Copy link
Contributor

eladb commented Feb 12, 2019

+1

Another thing we should enforce via awslint is that addXxx always returns void and new always returns a value.

Then, I was also thinking that we can implement deCDK heuristics for the "adds" since they can be represented as an array:

{
  capacity: [
    { "id": "machine", desiredCapacity: 5 } // kwargs at play
  ]
}

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Feb 12, 2019

Then, I was also thinking that we can implement deCDK heuristics for the "adds"

Do you mean "news"?

@eladb
Copy link
Contributor

eladb commented Feb 18, 2019

No, adds. We don't really need special heuristics for news because in those use cases users can simply define constructs in their template and reference them. For adds (like lambda.addEventSource), they need a way to specify them during initialization.

I was even thinking we can probably enforce this at the awslint level: if a construct has an addXxx method, then it's props must have an array xxx that can be used to declaratively specify those adds.

@eladb
Copy link
Contributor

eladb commented Feb 25, 2019

I am having second thoughts about new. I feel it "leaks" the implementation to the method name. Why do I care if this method creates another construct or not? The abstract concept is that I am adding something to my resource. The fact that there's another resource under the hood should be an implementation detail.

Why is it important to distinguish between "new" and "add"?

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Feb 25, 2019

Completely for user ergonomics and managing user expectations. The new variant:

  • Always takes an id parameter.
  • Always takes a props (or options) object.
  • Always returns the newly created object for further manipulation.

If we called everything add(), it could be one or the other, and users wouldn't know what to expect from the method name alone.

These kinds of conventions are even more important for untyped languages like JavaScript and Python, where the IDE (potentially) doesn't help as much.

@eladb
Copy link
Contributor

eladb commented Feb 28, 2019

I don't think that this is something we should/want to standardize actually. For example, id is not necessarily needed, if, for example, there's a safe way to define a unique ID for the construct based on some property of the new thing (for example, API gateway resources must be unique within their subtree so the resource name is used as the ID). Options is also not a must, sometimes it is more ergonomic to provide an API.

I think the only thing that makes these methods "special" is that they return an object, and this is captured in the method signature, so I don't really think it requires that the method name will encode this as well.

@eladb eladb added the @aws-cdk/core Related to core CDK functionality label Feb 28, 2019
@eladb eladb self-assigned this Aug 12, 2019
@eladb eladb added the package/awscl Cross-cutting issues related to the AWS Construct Library label Aug 28, 2019
@SomayaB SomayaB added the feature-request A feature should be added or improved. label Oct 26, 2019
@eladb eladb closed this as completed Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/core Related to core CDK functionality feature-request A feature should be added or improved. package/awscl Cross-cutting issues related to the AWS Construct Library
Projects
None yet
Development

No branches or pull requests

3 participants