digital-asset / daml-finance

Apache License 2.0
17 stars 16 forks source link

Some naming choices are clunky when working with Java #1032

Open matteolimberto-da opened 1 year ago

matteolimberto-da commented 1 year ago

Java doesn't allow import renaming so one can either use

The first does not work for things like Factory which will collide (Account.Factory, Instrument.Factory, ...).

The second leads to things like new com.daml.app.template.codegen.daml.finance.account.account.Factory(…) (the java codegen code usually has an additional package prefix).

Naming things like AccountFactory, HoldingFactory, … instead would make collisions much less likely and avoid the clunkiness of fully qualified imports. On the other hand, this makes imports in daml less neat (e.g. import D.F.Account.AccountFactory).

A potential solution could be to

An alternative is to provide a small companion Java library wrapping the fully qualified names.

Types for which name collision applies:

brianweir-da commented 11 months ago

This issue isn't exclusive to Daml Finance; it's a pervasive concern across all Daml applications utilizing the Java code generation. Consequently, addressing this problem extends beyond merely patching the Daml Finance library.

To ensure collision-free usage of the Daml Finance library in Java, it necessitates avoiding any clashes in interface, template, data types, or choice names. Implementing such changes is far from a minor refactoring endeavor. For instance, within the library, there are around 70 declarations of data View = View .... Enforcing name uniqueness throughout the library would lead to increasingly intricate and verbose identifiers. Drawing inspiration from Spring, we might end up with names akin to AbstractAnnotationConfigDispatcherServletInitializer. Such modifications could significantly impede the user experience for Daml development with this library.

Instead, I propose that our primary focus should be on devising a solution on the Java side of the equation. Once we've established a viable approach in that context, we can explore whether this pattern can be incorporated into the code generation process, thereby benefiting all projects that rely on it.

cocreature commented 11 months ago

@brianweir-da how are you suggesting we address this on the Java side? There isn't really any other option than generating prefixes automatically there and that will be super confusing. I'd much rather have library authors chose the names.

Enforcing name uniqueness throughout the library would lead to increasingly intricate and verbose identifiers.

You don't need full uniqueness, just make the common choices reasonable. E.g. switch to AccountFactory and FungibleFactory has imho no negative impact on the UX on the Daml side and it probably avoids 95% of the collisions here. Similar for the various View types.

Having fewer collisions in the names also leads to nicer autocompletion in vscode for typescript code so the problem isn't just Java specific.

brianweir-da commented 11 months ago

@brianweir-da how are you suggesting we address this on the Java side?

What I'm suggesting is that we focus on addressing this on the Java code-generation first. This problem isn't limited to Daml Finance. Afaik, any daml code where you have colliding names will run into this problem if you reference them in the same class in Java.

I'm open to switch to AccountFactory and FungibleFactory, but imo this is a tactical fix specific to your usage of this library.

filmackay commented 11 months ago

@brianweir-da is it worth having a play with the generated code and see what usability improvements you can make? This feels very focused on dev experience, so it would be good to see the intended experience being used in order to validate it.

I'm thinking particularly of nested static classes to create what looks like packages, but would allow you to do something like:

import com.daml.app.template.codegen.daml.finance.account.Account;

_ = new Account.Factory(...);

Ugly code to write, but we could codegen it 🤷🏻, but mainly I am just trying to use as-is names just in a more namespace-friendly way by exploiting packages/static classes.