search

btc-ag / service-idl

Xtext-based Service IDL (Interface Definition Language) and Code Generators for Protobuf, C++, Java and .NET
Eclipse Public License 2.0
8 stars 8 forks source link

Change derivation of release unit name: use main module instead of IDL file name #226

Closed sigiesec closed 5 years ago

sigiesec commented 5 years ago

Define implicit main module if none is specified explicitly Added some validation rules

Fixes #217

sigiesec commented 5 years ago

@huttenlocher Could you please give the comment in https://github.com/btc-ag/service-idl/blob/436956e0882651eedf77f2a7707c51a069dec5e1/com.btc.serviceidl/src/com/btc/serviceidl/generator/common/GeneratorUtil.xtend#L71 a look, in combination with the paths in the tests. I think it looks quite strange that there are paths such as BTC/PRINS/Infrastructure/ServiceHost/Demo/API.NET/Impl rather than BTC/PRINS/Infrastructure/ServiceHost/Demo/API/NET/Impl. Is this intended? If not, I would also change this in the scope of this PR. It was not as visible before, since the tests did not specify a main module, so there was no "NET" part added at all.

sigiesec commented 5 years ago

@kamiddel Could you test this version? (0.6.0-sigiesec-fix-issue-217-SNAPSHOT)

huttenlocher commented 5 years ago

I think it looks quite strange that there are paths such as BTC/PRINS/Infrastructure/ServiceHost/Demo/API.NET/Impl rather than BTC/PRINS/Infrastructure/ServiceHost/Demo/API/NET/Impl. Is this intended?

Yes, I think, this is intended, if API is marked as "main". Don't we have such naming schema in some CAB components as well, e.g. BTC/CAB/Crypto.NET/Impl etc.

sigiesec commented 5 years ago

Don't we have such naming schema in some CAB components as well, e.g. BTC/CAB/Crypto.NET/Impl etc.

Yes, there are such paths. But I think that this is for historic reasons only (they were in a single repository before migrating to Git).

sigiesec commented 5 years ago

@huttenlocher I decided to postpone this issue, and created https://github.com/btc-ag/service-idl/issues/234 to track it.

@huttenlocher Could you review the PR please? There are some possible interactions with the import additions of you and @pekueble, so it should be ready to merge soon.

kamiddel commented 5 years ago

@kamiddel Could you test this version? (0.6.0-sigiesec-fix-issue-217-SNAPSHOT)

Looks as expected