search

odpi / egeria

Egeria core
https://egeria-project.org
Apache License 2.0
810 stars 261 forks source link

Lombok - coding usability #5207

Closed planetf1 closed 3 years ago

planetf1 commented 3 years ago

Lombok has recently been added to Egeria.

Firstly, we have now lost the embedded javadoc that is being packaged and sent to maven central, which prevents debugging of egeria properly.

Secondly When compile errors, warnings, notifications occur at the CLI or in an IDE, the errors reported relate to the delomboked version of the code ie:

18:23:04,463 [WARNING] /Users/jonesn/IdeaProjects/egeria.maven/open-metadata-implementation/governance-servers/data-platform-services/data-platform-services-connector/target/delombok/org/odpi/openmetadata/dataplatformservices/api/model/DataPlatformDeployedDatabaseSchema.java:[5,68] org.odpi.openmetadata.accessservices.dataplatform.properties.DeployedDatabaseSchema in org.odpi.openmetadata.accessservices.dataplatform.properties has been deprecated

At least 3 developers in the last few weeks have then gone to fix the delomboked code, and got confused as to why the fix didn't work....

An explicit delombok is needed to correctly produce javadoc, but I would suggest this confusion is adding to the cognitive load of developers working on the platform which could trap new contributors to the platform.

I think we should:

  1. Remove lombok plugin activation from the top level pom and ONLY use it where it is CURRENTLY needed (I was pushing for lombok to be available to any component that need it so take some of the blame for not scoping it more narrowly, but at least we have discovered it's characteristics better)
  2. Propose that no further use of lombok is made until we can address the IDE usability issues If not consider removing lombok (the existing delombok’d source can be used for those modules currently using lombok annotations)
  3. Investigate if any build changes can mitigate this effect
  4. Make a decision on continuing to broaden lombok usage, maintain the status quo, or remove
planetf1 commented 3 years ago

Agreed in community call 20 Mar that we will go with #1 as a short term measure until we learn more about these issues (cc: @alexandra-bucur @lpalashevski @mandy-chessell @davidradl )

planetf1 commented 3 years ago

I plan to make change 1 today

alexandra-bucur commented 3 years ago

It's great that you spotted those behaviours. I would like to investigate them a bit in order to see what can be done. The framework adds a lot of benefits, so maybe we can find solutions. For the moment I think it's great that you start with point 1 from your proposal today. Thank you for doing that.

planetf1 commented 3 years ago

@alexandra-bucur I've done step 1 - assigning for you to look at any of the remaining aspects/see if we can reduce the impact before expanding usage further?

alexandra-bucur commented 3 years ago

@planetf1 I added the https://github.com/odpi/egeria/pull/5216 in the meantime in order to avoid the duplicate class errors in Intellij.

I will continue the investigation further for this issue :)

planetf1 commented 3 years ago

@planetf1 I added the #5216 in the meantime in order to avoid the duplicate class errors in Intellij.>

Added a note to the PR review -- this invocation of clean appeared to be breaking our javadoc generation, and with plugin installed I did not see dup class errors. see PR for longer comment

planetf1 commented 3 years ago

Taking a step back, we are trying lombok to reduce the overhead of writing and maintaining standard boilerplate code.

We could also look at either Google's 'AutoValues' or Immutables. Both work with standard java annotation processing which I think will fit more smoothly with regular dev tools. Lombok is a bit of an outlier due to it's bytecode manipulation ? I've not looked closely though nor checked to see how Javadoc may be handled. Nor indeed any other downsides...

AutoValues - https://github.com/google/auto/tree/master/value Immutables - https://immutables.github.io/immutable.html

planetf1 commented 3 years ago

I had an instance of the IntelliJ error today, using the very latest 2021.1.2 RC (EAP) with the lombok plugin active:

/Users/jonesn/IdeaProjects/egeria.maven/open-metadata-implementation/access-services/asset-lineage/asset-lineage-api/target/delombok/org/odpi/openmetadata/accessservices/assetlineage/ffdc/AssetLineageErrorCode.java:28:8
java: duplicate class: org.odpi.openmetadata.accessservices.assetlineage.ffdc.AssetLineageErrorCode

Whilst I normally only compile using the 'maven' option even within IntelliJ, today I looked at the IDE's support for module-info files, which caused the IDE to use it's own internal builder.. and hit this issue. Perhaps co-incidental, but this is probably also a scenario where a 'clean' task wouldn't help anyway since maven was not invoked here.

(I happened to be in AssetDescription.java and clicked on menu-> Code -> Generate module-info descriptors )

planetf1 commented 3 years ago

I hit this same problem again yesterday when refactoring , this time in data engine OMAS. IntelliJ (up to date, with plugin) was reporting dup classes in data-engine-api even though maven/cli was fine. This is restricted to a small set of components for now.

alexandra-bucur commented 3 years ago

@planetf1 I managed to solve the duplicate class issue as described in https://github.com/odpi/egeria/pull/5216. Please see if the first comment in the conversation helps you restore your environment. On my side it looks good :)

alexandra-bucur commented 3 years ago

Taking a step back, we are trying lombok to reduce the overhead of writing and maintaining standard boilerplate code.

We could also look at either Google's 'AutoValues' or Immutables. Both work with standard java annotation processing which I think will fit more smoothly with regular dev tools. Lombok is a bit of an outlier due to it's bytecode manipulation ? I've not looked closely though nor checked to see how Javadoc may be handled. Nor indeed any other downsides...

AutoValues - https://github.com/google/auto/tree/master/value Immutables - https://immutables.github.io/immutable.html

@planetf1 unfortunately these 2 alternatives are both limited to immutable classes which is not the case for us. I could't find a better alternative for our use case.