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

consider renaming the "id" parameter for constructs (reserved word in Python) #3203

Closed
1 of 5 tasks
bverhoeve opened this issue Jul 4, 2019 · 5 comments
Closed
1 of 5 tasks
Assignees
Labels
@aws-cdk/core Related to core CDK functionality effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. language/python Related to Python bindings

Comments

@bverhoeve
Copy link

bverhoeve commented Jul 4, 2019

Note: for support questions, please first reference our documentation, then use Stackoverflow. This repository's issues are intended for feature requests and bug reports.

  • I'm submitting a ...

    • 🪲 bug report
    • 🚀 feature request
    • 📚 construct library gap
    • ☎️ security issue or vulnerability => Please see policy
    • ❓ support request => Please see note at the top of this template.
  • What is the current behavior?
    To create a new Construct (or any other Resource) using CDK, the constructor in Python has the following syntax:

 class aws_cdk.core.Construct(scope, id)

The id parameter serves as a unique id for CloudFormation. Naming a parameter id in Python is not really good practice, since id() is a built-in function in the Python standard library, which represents the address of the object in memory in CPython. (see https://docs.python.org/3/library/functions.html#id)

I think this could lead to potentials errors down the line for users that use the Python variant of CDK, since the id parameter could clash with the id() function.

  • What is the expected behavior (or behavior of feature suggested)?
    If possible, renaming the id parameter to something like identifier or cdk_id would avoid having this parameter clash with the built-in id() function.

  • What is the motivation / use case for changing the behavior or adding this feature?
    Clashes with Python standard library built-in functions.

  • Please tell us about your environment:

    • CDK CLI Version: 0.36.2
    • Module Version: 0.36.2.0
    • OS: [all]
    • Language: [Python ]
  • Other information (e.g. detailed explanation, stacktraces, related issues, suggestions how to fix, links for us to have context, eg. associated pull-request, stackoverflow, gitter, etc)

@bverhoeve bverhoeve added the needs-triage This issue or PR still needs to be triaged. label Jul 4, 2019
@SomayaB SomayaB added language/python Related to Python bindings @aws-cdk/core Related to core CDK functionality labels Oct 11, 2019
@SomayaB
Copy link
Contributor

SomayaB commented Oct 11, 2019

Hi @bverhoeve, thanks for submitting this request! We will look into this and someone will update this issue when there is movement.

@SomayaB SomayaB added feature-request A feature should be added or improved. and removed needs-triage This issue or PR still needs to be triaged. labels Oct 11, 2019
@eladb eladb added the effort/medium Medium work item – several days of effort label Jan 23, 2020
@eladb eladb changed the title id parameter for Constructs possible errors in Python consider renaming the "id" parameter for constructs (reserved word in Python) Jan 23, 2020
@eladb eladb mentioned this issue Jan 23, 2020
12 tasks
@eladb eladb closed this as completed Aug 17, 2020
@SamStephens
Copy link
Contributor

@eladb Look through the linked issues, I cannot see any discussion of this request, it appears to have been closed with no discussion. It'd be great to understand the motivations behind not doing this.

@bazd
Copy link

bazd commented Dec 6, 2021

@eladb Look through the linked issues, I cannot see any discussion of this request, it appears to have been closed with no discussion. It'd be great to understand the motivations behind not doing this.

I just tested this with CDK 2.0.0 and the sample-app, and looks like they have renamed it to 'construct_id'.

class HelloCdkStack(Stack):
def init(self, scope: Construct, construct_id: str, **kwargs) -> None:
super().init(scope, construct_id, **kwargs)

@SamStephens
Copy link
Contributor

@bazd that may be the case in the sample application, but the actual CDK constructs themselves still use the keyword id. See for example the constructor of Stack that's being called above via super.init. If you rewrote that code sample to use named parameters, it'd be:

class HelloCdkStack(Stack):
    def init(self, scope: Construct, construct_id: str, **kwargs) -> None:
        super().init(
            scope=scope,
            id=construct_id,
            **kwargs,
        )

@bazd
Copy link

bazd commented Dec 7, 2021

@bazd that may be the case in the sample application, but the actual CDK constructs themselves still use the keyword id. See for example the constructor of Stack that's being called above via super.init. If you rewrote that code sample to use named parameters, it'd be:

class HelloCdkStack(Stack):
    def init(self, scope: Construct, construct_id: str, **kwargs) -> None:
        super().init(
            scope=scope,
            id=construct_id,
            **kwargs,
        )

Yes, cheers for clarifying. They haven't addressed it at all which is disappointing considering v2 has just been released.

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 effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. language/python Related to Python bindings
Projects
None yet
Development

No branches or pull requests

5 participants