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

When a JSII interface has a name like IFoo, the generated .NET interface is named IIFoo #109

Closed
mpiroc opened this issue Jul 30, 2018 · 9 comments · Fixed by #728
Closed
Assignees
Labels
effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. language/dotnet Related to .NET bindings (C#, F#, ...) needs-discussion This issue/PR requires more discussion with community. p0

Comments

@mpiroc
Copy link
Contributor

mpiroc commented Jul 30, 2018

We should detect the case where the interface already has the I- prefix, and skip adding an additional I- prefix in this case.

Open question: What if the interface is named something like IAMFoo? I.e. it begins with I<capital letter>, but the I isn't intended as a prefix.

@mpiroc mpiroc added language/dotnet Related to .NET bindings (C#, F#, ...) enhancement labels Aug 11, 2018
@srchase srchase added feature-request A feature should be added or improved. and removed enhancement labels Jan 3, 2019
@fulghum fulghum added the p0 label Apr 2, 2019
@fulghum fulghum added the effort/large Large work item – several weeks of effort label Apr 22, 2019
@RomainMuller RomainMuller added this to the .NET Support milestone Jul 30, 2019
@assyadh
Copy link
Contributor

assyadh commented Aug 12, 2019

After talking with Romain, we chose to:

Not prefix with I anymore for interfaces that are NOT datatype (jsii already enforces that non-datatype interfaces have a name prefixed with the I). Only datatype interfaces should append an I.

@eladb
Copy link
Contributor

eladb commented Aug 12, 2019

Do we really need interfaces for data types? I was under the impression that in .NET we can express these data types without interfaces.

@assyadh
Copy link
Contributor

assyadh commented Aug 12, 2019

See my comment here:

https://github.com/aws/jsii/blob/master/packages/jsii-pacmak/lib/targets/dotnet/dotnetgenerator.ts#L148

Right now for each interface we create the interface, the interface proxy and the interface datatype class (implementing the original interface).

In the case of a datatype (props), we could do with JUST the datatype, not implementing any interface.

Relates: #480

Given that props objects are datatypes, they would live by themselves.

@eladb
Copy link
Contributor

eladb commented Aug 13, 2019

So if I understand correctly the answer is that we can get rid of interfaces for data types (formally known as “structs”). Correct?

@assyadh
Copy link
Contributor

assyadh commented Aug 13, 2019

Yes

@fulghum
Copy link

fulghum commented Aug 13, 2019

@assyadh – so the work for this task now, is just to remove the generation of interfaces for struct/data classes?

@fulghum fulghum added effort/small Small work item – less than a day of effort and removed effort/large Large work item – several weeks of effort labels Aug 13, 2019
@fulghum fulghum added the needs-discussion This issue/PR requires more discussion with community. label Aug 13, 2019
@assyadh
Copy link
Contributor

assyadh commented Aug 13, 2019

After discussion with Romain at the bindings meeting, looks like the interfaces are here to stay. There is usecases of interfaces implementations that are allowed by the language. Removing them would be a deal breaker.

I'll let @RomainMuller chime in on the specifics.

@RomainMuller
Copy link
Contributor

The current state of things is that data types support multiple inheritance, which implies supporting interfaces in dotnet. Have to check whether we can disallow multiple inheritance, which would render interfaces unnecessary.

@assyadh
Copy link
Contributor

assyadh commented Aug 20, 2019

After discussion with @RomainMuller , we can remove the added I in the interfaces as it JSII is already enforcing the I for an interface.

RomainMuller pushed a commit that referenced this issue Aug 22, 2019
…728)

Drop the useless I prefix for non datatype interfaces as they are guaranteed by jsii to start with I.

This removes a bunch of II* interfaces.

Updated the tests, and tested with the CDK successfully.

BREAKING CHANGE: names of .NET behavioral interfaces have changed (the duplicate prefix I was removed).

Fixes #109
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. language/dotnet Related to .NET bindings (C#, F#, ...) needs-discussion This issue/PR requires more discussion with community. p0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants