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

refactor(document-handling): Hacking session split document modules #3821

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mathias-vandaele
Copy link
Contributor

Description

Hacking session on document handling

Related issues

closes #

Checklist

  • PR has a milestone or the no milestone label.

@mathias-vandaele mathias-vandaele requested a review from a team as a code owner December 19, 2024 13:57
@mathias-vandaele mathias-vandaele added this to the 8.7.0-alpha3 milestone Dec 19, 2024
@mathias-vandaele mathias-vandaele self-assigned this Dec 19, 2024
@mathias-vandaele mathias-vandaele changed the title Hacking session split document modules refactor(document-handling): Hacking session split document modules Dec 19, 2024
johnBgood
johnBgood previously approved these changes Dec 19, 2024
Copy link
Collaborator

@johnBgood johnBgood left a comment

Choose a reason for hiding this comment

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

Just some questions, otherwise great

@@ -59,7 +59,8 @@ public FeelEngineWrapper() {
.registerModule(DefaultScalaModule$.MODULE$)
.registerModule(new JavaTimeModule())
.registerModule(new Jdk8Module())
.registerModule(new JacksonModuleDocument(new DocumentFactoryImpl(null), null))
.registerModule(
new JacksonModuleDocumentDeserializer(new DocumentFactoryImpl(null), null))
Copy link
Collaborator

Choose a reason for hiding this comment

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

we might need the serializer only righ?


@Override
public String getModuleName() {
return "JacksonModuleDocument";
Copy link
Collaborator

Choose a reason for hiding this comment

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

we need to update the name, same for the other module

@Override
public Version version() {
// TODO: get version from pom.xml
return new Version(0, 1, 0, null, "io.camunda", "jackson-datatype-document");
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here

@sbuettner
Copy link
Contributor

Looks good, can you exclude all files not relevant to the document module changes?

@mathias-vandaele
Copy link
Contributor Author

Looks good, can you exclude all files not relevant to the document module changes?

Hey @sbuettner, it was said during the hacking session that we could be doing both same times, but you are right, to be cleaner, let's separate both PR

@mathias-vandaele mathias-vandaele force-pushed the hacking-session-split-document-modules branch from 02666be to 401df28 Compare December 20, 2024 10:38
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.

3 participants