jsii icon indicating copy to clipboard operation
jsii copied to clipboard

@jsii.implements should error when class doesn't implement interface

Open rix0rrr opened this issue 3 years ago • 1 comments

:rocket: Feature Request

Affected Languages

  • [ ] TypeScript or Javascript
  • [x] Python
  • [ ] Java
  • [ ] .NET (C#, F#, ...)
  • [ ] Go

Description

@jsii.implements marks a class as implementing an interface (maybe?), but it doesn't check for completeness.

Example:

from aws_cdk import(
    pipelines,
	aws_codepipeline_actions as cpactions,
	aws_codepipeline as codepipeline
)
import jsii

@jsii.implements(pipelines.ICodePipelineActionFactory)
class LambdaApprovalStep(pipelines.Step):
	def __init__(self, id_, some_lambda):
		super().__init__(id_)

		@jsii.member(jsii_name="produceAction")		
		def produce_action(
			self, stage: codepipeline.IStage,
			options: pipelines.ProduceActionOptions
		) -> pipelines.CodePipelineActionFactoryResult:
			stage.add_action(
				cpactions.LambdaInvokeAction(
					lambda_ = some_lambda
				)
			)
			return pipelines.CodePipelineActionFactoryResult(run_orders_consumed=1)

This code has a mistake because the def produce_action occurs at the wrong level. It's nested inside the constructor, instead of on the class.

@jsii.implements() could have noticed the class isn't completely correctly that interface, and thrown an error. That would have allowed catching the error early, and at class definition time, instead of what's happening now:

jsii.errors.JSIIError: Deployment step 'Step(SomeLambdaStep)' is not supported for CodePipeline-backed pipelines

That error is caused because the Step object fails the following check:

function isCodePipelineActionFactory(x: any): x is ICodePipelineActionFactory {
  return !!(x as ICodePipelineActionFactory).produceAction;
}

Which means there is no produceAction method on the object... so what is @jsii.implements even doing?

Proposed Solution

Have @jsii.implements check members that ought to be implemented and throw an error if they aren't.

rix0rrr avatar Jan 19 '22 09:01 rix0rrr

Thanks @rix0rrr , this is a good addition, and it most definitely would have helped catch it early.

mrpackethead avatar Jan 19 '22 19:01 mrpackethead