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

DON'T Merge Feature/AxonFramework (use #123, #124, #125) #120

Closed
wants to merge 8 commits into from

Conversation

zambrovski
Copy link
Contributor

@zambrovski zambrovski commented Jun 3, 2022

Hi Oliver,

here is a PR implementing the Axon Framework support. I would be happy for any feedback on this, since I believe this feature is very valuable (for all Axon users).

Current

  • Updated pom.xml with to use latest Maven Plugins, set enforced JDK to 17 (is LTS), add dependencies to Axon Framework
  • Add more helpers to PluginSupport and JMoleculesElementMatchers
  • Implemented AxonFramework and Axon Spring bytebuddy plugins
  • Added an example with an Axon Test (as a separate project) to test that the annotation really work

Temporary

  • Added "local" Association annotation until 1.6 containing is released

TODO / Missing:

Fixes:
#121

Cheers,

Simon

@odrotbohm
Copy link
Member

This looks very well already! Would you mind applying the formatting settings you find here to the PR? It looks like the changeset contains a lot of formatting changes and result in a wild mixture of tabs and spaces.

Does Axon require Java 17 already? As we would like to stay compatible with Boot 2.x, we need to generally be Java 8 compatible with the source code we produce. If the Axon JARs already require Java 17, we, of course, can upgrade the build script to make sure it uses a JDK 17 to build.

@zambrovski
Copy link
Contributor Author

zambrovski commented Jun 3, 2022

Sorry for bad formatting, will apply this.

Java 17 - no for axon it is just fine to run on JDK 8+. In jmolecules-integrations parent pom you had an enforcer plugin forcing me to have exactly JDK 15 (which I don't have on my machine), so I decided not to remove it entirely, but to upgrade to latest LTS (17). It is up to you what to use, usually I enforce to use 11+ - the previous LTS and most popular as far as I can see from other projects I'm in...

I just tried it and found out that we indeed require 15+, so I changed the enforced range to [15,). It will now continue to run on 15 and all above.

BTW: I'm using jEnv to ease the development. It works fine for Linux, Windows and Mac and puts a small file (.jenv) inside the project to specify the version. There are also plugins for IDEs taking it into account. This makes it easy to have multiple JDKs on your machine and configure it on a per-project basis.

@odrotbohm
Copy link
Member

Hm, there are still quite a few sole formatting changes that clutter the diff. I'd also like to clearly separate changes that are nice but actually unrelated to the actual functionality (updating the Maven compiler plugin version or the use of .jenv) from the actual changes. I'd even argue, they shouldn't be in this PR at all.

The example project could use Lombok to avoid a lot of boilerplate needing to be read and understood.

I like the idea of centralizing the filtering of platform types (Java, Kotlin etc.). I'd even go ahead and make that a dedicated method to avoid having to expose the list of values. Right now it's only handed into a method on the very same type that exposes the list in the first place. That feels like unnecessary ceremony.

I don't want to drag this out for you as the core parts of the contribution are very valuable and I don't want to waste your time. If you'd like, I just can take it from here and polish stuff up as we need it.

@zambrovski
Copy link
Contributor Author

zambrovski commented Jun 3, 2022

Tell me how I can help. So I can separate the request in several.

My approach of grouping would be:

What do you think?

Cheers,

Simon

@odrotbohm
Copy link
Member

Sounds good to me! And thanks again for your patience! 🙃

@zambrovski
Copy link
Contributor Author

Instead of PR #120 I created #122, #123 and #124 ...
I think it is easier now to understand the changes.

I hope this way we get it done soon.

Btw. have you seen the issues I opened in xmolecules/jmolecules? The xmolecules/jmolecules#83 and xmolecules/jmolecules#84 are trivial (more like bugs), but the issue about the query is more a conceptional question.

@zambrovski zambrovski changed the title Feature/AxonFramework DON'T Merge Feature/AxonFramework (use #123, #124, #125) Jun 4, 2022
@odrotbohm odrotbohm closed this Jun 9, 2022
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