microsoft / kiota

OpenAPI based HTTP Client code generator
https://aka.ms/kiota/docs
MIT License
3.03k stars 210 forks source link

Naming Convention and Alignment, DotNet / Java #3545

Closed ramsessanchez closed 1 year ago

ramsessanchez commented 1 year ago

While working on the java snippet generation for Graph I noticed a slight deviation from the expected output of our class naming in Java. In particular, I noticed that our folder names for java maintain underscores when generated (in our Graph example we have 'security/alerts_v2'), however, the class names, properties, and method names omit them (in our same Graph example we generate the class alertsV2RequestBuilder and the method alertsV2). I believe this was a part of the work done in the Java refiner recently. The naming of the folder should also reflect the methods / properties that it will enclose. In #3535 I begun the work to maintain the underscores so that we generate alerts_v2() and alerts_v2RequestBuilder so that it reflects the folder name and matches what is going on in dotNet. Upon further reflection I believe that we should remove the underscores from folders and have them match the naming of the properties and methods as they currently are in Java. This would present a slight deviation from dotNet where we currently are keeping underscores across the board for properties, methods, and folders. For Graph in particular I know that we wanted to keep namings very similar for Java and Dotnet and this would go against that. I do not know if the decision to keep underscores in dotnet was deliberate so I am wondering if Java should match or not.
I am of the opinion that underscores should be removed from the folder names in Java to match the classes that we generate even though that would differ from dotNet. I also think that dotNet should consider doing the same in the next major version of Kiota although I worry that for the dotNet Graph sdk this would also be a breaking change and would require a major version bump as well. For java we are on the cusp of releasing the next version of the graph sdk so a breaking change is not as much of a concern fortunatley. I would really appreciate any added thoughts and opinions as well as discussing whether there should be set naming standards across all languages.

baywet commented 1 year ago

Thanks for starting this conversation

Let's try to recap the current situation in terms of naming conventions for namespaces:

language keeps underscore in namespace
CSharp yes
Java TBD
Go yes
TypeScript yes
Python yes, but snake case convention
Ruby yes, but snake case convention
PHP yes
CLI same as CSharp

Normalizing this would represent a breaking change for Go and CSharp for the time being (and Python/PHP if we don't do it quickly enough).

Since that normalization:

I'm fine with this being different on a per language basis : we could decide to normalize that aspect for each language. For languages that are already stable, we could make this normalization conditional to the "ExcludeBackwardCompatible" new switch.

Thoughts @andreaTP @samwelkanda @Ndiritu @SilasKenneth @andrueastman @rkodev ?

andreaTP commented 1 year ago

Using underscores in Java packages is perfectly fine (even encouraged in some circumstances): https://docs.oracle.com/javase/tutorial/java/package/namingpkgs.html

Instead a class name alerts_v2RequestBuilder sounds alien and it should probably become AlertsV2RequestBuilder.

I am of the opinion that underscores should be removed from the folder names in Java to match the classes that we generate even though that would differ from dotNet.

I do not share the same sentiment as it will generate less idiomatic Java code.

ramsessanchez commented 1 year ago

@andreaTP I get what you're saying. My worry is less so about the underscores and more so about the inconsistency in naming between the package and the request builder enclosed as the convention in kiota has been to have the folder name as the prefix for the request builder. Given that underscores are allowed for packages in Java, we keep them in all other languages, and it is the current implementation then I am perfectly fine keeping as is, but I believe the naming should remain consistent such that the class name should also maintain the underscore as it is also perfectly allowed in java.

andreaTP commented 1 year ago

Hey @ramsessanchez , thanks for getting back!

Underscores are allowed in class names, but they are usually used only in extreme cases where any other heuristics fails from my personal experience. What I'm saying is that we are going to generate correct but not idiomatic code.

I think here we can make use of some other Java users to break the tie(feel free to involve other ppl as well!).

Cc. @EricWittmann

EricWittmann commented 1 year ago

In my experience underscores are uncommon in Java but not unheard of, especially for generated code. So I do agree with @andreaTP that generating package and classnames with underscores isn't very Java idiomatic. But it's not going to ruin anyone's year either. To have an opinion on this I would ask:

  1. Can a developer simply avoid using underscores in their OpenAPI definitions? Or will there always be one due to some sort of prefixing?
  2. How valuable is consistency between Java and Dotnet? Does the consistency help with something?

Note: I will add that generated class names should absolutely be capitalized. If a code generator produced a Java class called alerts_v2RequestBuilder I would be quite annoyed. :)

baywet commented 1 year ago

Can a developer simply avoid using underscores in their OpenAPI definitions? Or will there always be one due to some sort of prefixing?

In the case of Java, kiota is not adding underscores, the only ones that will be there will be coming from the description.

How valuable is consistency between Java and Dotnet? Does the consistency help with something?

Nice to have but not primordial.

alerts_v2RequestBuilder I would be quite annoyed

I believe Ramses was referring to the namespace segment and the java file name but the class name will have a capital A.

Overall, I'd vote to keep the namespace/class name as is as it matches the path segment, and as annoying as it is, if somebody has both "alerts_v2" and "alertsv2" for some reason, removing the underscore will lead to collisions.

andreaTP commented 1 year ago

Overall, I'd vote to keep the namespace/class name as is as it matches the path segment

+1 from my side

ramsessanchez commented 1 year ago

Thanks for the feedback, everyone! To summarize, if we keep the namespace/class name as is, which would match the path segment, the following will change (for the sake of the example I am using the alerts_v2 example):

namespace: security/alerts_v2 will remain the same.

class name: security/alerts_v2/AlertsV2RequestBuilder.java will change to security/alerts_v2/Alerts_V2RequestBuilder.java

method name: public AlertsV2RequestBuilder alertsV2() will change to public Alerts_V2RequestBuilder alerts_V2()

Let me know if any of this is incorrect or if I misunderstood anything. Thanks!

andreaTP commented 1 year ago

Thanks @ramsessanchez for the feedback!

I think that keeping the current encoding is best, as the namespace is already disambiguating the class. Having class names without unneeded underscores is pretty good from the idiomatic pov of Java developers imho.

baywet commented 1 year ago

Perfect, I think we're all on the same page and we can close both this issue and #3535. Closing.