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

Revision meta data #137

Merged
merged 4 commits into from
Aug 31, 2019
Merged

Revision meta data #137

merged 4 commits into from
Aug 31, 2019

Conversation

vapadwal
Copy link
Member

@vapadwal vapadwal commented Aug 18, 2019

Fix for Issue : - #47 .
This is new PR created against PR #134

Copy link
Member

@hohwille hohwille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vapadwal Thanks for taking over this PR and improving 👍
This is now nice rearding the API. Can you generate the JavaDoc and check if it is sane? AFAIK JavaDoc taglets are case sensitive and @deprecated in JavaDoc has to be lower-case.

However, it is still not working as planned yet.
See the APIs and implementation here:

It is still producing the old depreated code which is unrealted to the new one. Therefore the new API can not yet be used.

My proposal would be:
Let the old and deprecated code extend the new API such that the old deprected interface and class are more or less empty and just inherit from the new API types.
For compatibility we could go these steps:

  1. keep the DAO/Repositories untouched for the moment (using deprecated code).
  2. push out a release like this
  3. let projects update to that release
  4. now projects can update their code such that they use the new API and remove references to the deprecated classes from their code.
  5. For the next devon4j release we then remove the deprecated classes and only use the new APIs.
  6. Projects that did step 4. are now safe to upgrade to 5.

An alternative would be to simplify and go this way:

  • we give up compatibility and remove the old code (instead of deprecating it)
  • we directly refactor DAO/Repository to the new API
  • we provide support in devon-ide for updating projects in our migrator

I would vote that we have migration support for our projects regarding this change anyhow. So maybe the latter approach would be much simpler. I am not entirely sure how our projects deal with breaking changes and if they are all aware of devon-ide and its migration feature.

@hohwille hohwille added this to the release:3.2.0 milestone Aug 19, 2019
@vapadwal
Copy link
Member Author

@vapadwal Thanks for taking over this PR and improving 👍
This is now nice rearding the API. Can you generate the JavaDoc and check if it is sane? AFAIK JavaDoc taglets are case sensitive and @deprecated in JavaDoc has to be lower-case.

However, it is still not working as planned yet.
See the APIs and implementation here:

It is still producing the old depreated code which is unrealted to the new one. Therefore the new API can not yet be used.

My proposal would be:
Let the old and deprecated code extend the new API such that the old deprected interface and class are more or less empty and just inherit from the new API types.
For compatibility we could go these steps:

  1. keep the DAO/Repositories untouched for the moment (using deprecated code).
  2. push out a release like this
  3. let projects update to that release
  4. now projects can update their code such that they use the new API and remove references to the deprecated classes from their code.
  5. For the next devon4j release we then remove the deprecated classes and only use the new APIs.
  6. Projects that did step 4. are now safe to upgrade to 5.

An alternative would be to simplify and go this way:

  • we give up compatibility and remove the old code (instead of deprecating it)
  • we directly refactor DAO/Repository to the new API
  • we provide support in devon-ide for updating projects in our migrator

I would vote that we have migration support for our projects regarding this change anyhow. So maybe the latter approach would be much simpler. I am not entirely sure how our projects deal with breaking changes and if they are all aware of devon-ide and its migration feature.

I was able to generated javadoc with it.
I have selected first approach as it will get time for the projects to deal with the breaking changes as in 3.2.0 release they will also be new to devon-ide

Copy link
Member

@hohwille hohwille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use Eclipse refactoring features. Otherwise you should search References on Workspace on the old code in order to manually refactor the code to ensure a working result.
Here are my hits (but do not use me as "refactoring service" - I am just working in the browser on github what does not mean I am not missing places, this is just what I was missing in the diffs and why I notice it can not work this way. It would be awesome if we had JUnits for all these features.):

https://github.com/vapadwal/devon4j/blob/9b8f9461997bb5d8048a9fec42bec3625652eec3/modules/jpa-basic/src/main/java/com/devonfw/module/jpa/dataaccess/impl/LazyRevisionMetadata.java#L9

https://github.com/vapadwal/devon4j/blob/9b8f9461997bb5d8048a9fec42bec3625652eec3/modules/jpa-spring-data/src/main/java/com/devonfw/module/jpa/dataaccess/impl/data/GenericRevisionedRepositoryImpl.java#L18

https://github.com/vapadwal/devon4j/blob/9b8f9461997bb5d8048a9fec42bec3625652eec3/modules/jpa-envers/src/main/java/com/devonfw/module/jpa/dataaccess/base/AbstractGenericRevisionedDao.java#L15

Copy link
Member

@hohwille hohwille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vapadwal now the refactoring is consistent. 👍 Thanks. All is looking good but I have still one remark for improvement. Please see my last review comment and provide a final fix.
Thanks in advance...

* @return die Instanz von {@link RevisionMetadataType} bzw. {@code null} falls {@code revision} den Wert {@code null}
* hat.
*/
public static RevisionMetadataType of(AdvancedRevisionEntity revEntity) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The static of method is typically used as a factory method of the same class defining the static method (as a short form for valueOf what is used inside the JDK as convention/pattern).
In this case JpaHelper.of(entity) the API is IMHO very confusing. You need to move the method to RevisionMetadataType. In case it should remain in JpaHelper you should rename this of method to asRevisionMetadata (like e.g. asEntity).

@hohwille
Copy link
Member

Just to be aligned. I understand the latest state of this PR such that:

  • We keep the existing classes deprecated in 3.2.0 as a kind of pointer and documentation for users to simplify manually updating.
  • We still need to provide a migration in devon-ide: Create devon4j migration for RevisionMetadata  ide#237
  • We will remove the deprecated classes in 3.3.0.

Copy link
Member

@hohwille hohwille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vapadwal 👍 now everything looks perfect to me. Thanks!

@hohwille hohwille merged commit 1b73a8f into devonfw:develop Aug 31, 2019
This was referenced Aug 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants