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

Generate type validation code #1321

Closed
3 of 6 tasks
iliapolo opened this issue Mar 9, 2020 · 5 comments
Closed
3 of 6 tasks

Generate type validation code #1321

iliapolo opened this issue Mar 9, 2020 · 5 comments
Labels
closed-for-staleness effort/large Large work item – several weeks of effort feature-request A feature should be added or improved. p2

Comments

@iliapolo
Copy link
Contributor

iliapolo commented Mar 9, 2020

🚀 Feature Request

Add type validation code to dynamic languages.

Affected Languages

  • TypeScript or Javascript (Javascript)
  • Python
  • Java
  • .NET (C#, F#, ...)

General Information

  • JSII Version: 0.22.0 (build 14afdde), typescript 3.7.5
  • Platform: Darwin a483e7b9e4e1.ant.amazon.com 18.7.0 Darwin Kernel Version 18.7.0: Thu Jan 23 06:52:12 PST 2020; root:xnu-4903.278.25~1/RELEASE_X86_64 x86_64
  • I may be able to implement this feature request
  • This feature might incur a breaking change

Description

We currently have quite a few issues coming from users who mistakenly pass L1 objects to L2 constructs.

For example: aws/aws-cdk#6540

These types of errors are not presented in typed languages because they actually cause a compile time error. It would be nice to provide a similar experience in dynamic languages as well.

Proposed Solution

JSII can generate some sort of type validation code in the proxy objects, something like:

@jsii.data_type(jsii_type="@aws-cdk/aws-cloudfront.S3OriginConfig", jsii_struct_bases=[], name_mapping={'s3_bucket_source': 's3BucketSource', 'origin_access_identity': 'originAccessIdentity'})
class S3OriginConfig():
    def __init__(self, *, s3_bucket_source: aws_cdk.aws_s3.IBucket, origin_access_identity: typing.Optional["IOriginAccessIdentity"]=None):
        """S3 origin configuration for CloudFront.

        :param s3_bucket_source: The source bucket to serve content from.
        :param origin_access_identity: The optional Oriagin Access Identity of the origin identity cloudfront will use when calling your s3 bucket. Default: No Origin Access Identity which requires the S3 bucket to be public accessible

        stability
        :stability: experimental
        """

        # type validation
        if not isinstance(origin_access_identity, IOriginAccessIdentity):
            raise RuntimeError('Illegal argument type...)

        self._values = {
            's3_bucket_source': s3_bucket_source,
        }
        if origin_access_identity is not None: self._values["origin_access_identity"] = origin_access_identity
@iliapolo iliapolo added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Mar 9, 2020
@RomainMuller RomainMuller self-assigned this Mar 9, 2020
@RomainMuller RomainMuller added effort/medium Medium work item – a couple days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Mar 9, 2020
@RomainMuller
Copy link
Contributor

I don't think this is a breaking change (it'd prohibit cases that won't have worked).

I also believe we should start off by issuing warnings instead of breaking the run-time (which makes it even less breaking). Possibly have an environment-controlled "strict" mode that throws instead of warning.

@iliapolo
Copy link
Contributor Author

iliapolo commented Mar 9, 2020

I don't think this is a breaking change (it'd prohibit cases that won't have worked).

Isn't it possible that there exists cases with "accidental" compatibility? Where the property names just happen to match?

I also believe we should start off by issuing warnings

I agree this would prevent the breakage, at the first iteration anyway.

@eladb
Copy link
Contributor

eladb commented Mar 9, 2020

dup with #75?

@RomainMuller
Copy link
Contributor

@eladb - it's related, but I think adding checks in Python code can be delivered faster and stop the "bleeding".

@RomainMuller RomainMuller added effort/large Large work item – several weeks of effort p2 and removed effort/medium Medium work item – a couple days of effort labels Aug 13, 2020
@RomainMuller RomainMuller removed their assignment Jun 24, 2021
@github-actions
Copy link
Contributor

This issue has not received any attention in 2 years. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added closing-soon This issue will automatically close in 4 days unless further comments are made. closed-for-staleness and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Jun 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-for-staleness effort/large Large work item – several weeks of effort feature-request A feature should be added or improved. p2
Projects
None yet
Development

No branches or pull requests

3 participants