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.

Sign up for GitHub

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

Jump to bottom

DROOLS-5249: do not store DateTimeFormatter as free form line #2859

Merged
merged 1 commit into from
May 21, 2020
Merged

DROOLS-5249: do not store DateTimeFormatter as free form line #2859

merged 1 commit into from
May 21, 2020

Conversation

jomarko
Copy link
Contributor

@jomarko jomarko commented Apr 16, 2020 •
edited
Loading

DROOLS-5249: do not store DateTimeFormatter as free form line

We have two kinds of date boiler plate code snippets. For:

  • java.util.Date: 'java.text.SimpleDateFormat sdf'
  • java.time.LocalDate: 'java.time.format.DateTimeFormatter dtf'

If user set value either of 'Date' or 'LocalDate' field using 'Literal Value' guided editor action, the given date boiler plate is generated automatically behind the scene. This allows users to put in just value of date, e.g. "01-Jan-2000". User do not have to use parametric constructors, builders, parsers or factory methods. It is done behind the scene using 'sdf' or 'dtf' BoilerPlate.

If users set value either of 'Date' ot 'LocalDate' field using 'Formula' guided editor action, is their responsibility to construct 'Date' or 'LocalDate' instance.

For more details see https://issues.redhat.com/browse/DROOLS-5249

@jomarko jomarko requested review from Rikkola and dupliaka April 16, 2020 12:57
@jomarko
Copy link
Contributor Author

jomarko commented Apr 16, 2020

jenkins execute full downstream build

dupliaka
dupliaka suggested changes Apr 17, 2020
View reviewed changes
Copy link
Contributor

@dupliaka dupliaka left a comment

Choose a reason for hiding this comment

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

I tested it manually and it appears that now instead of not showing twice the Date Time Formatter is not showing at all at first glance.
image

Rikkola
Rikkola requested changes Apr 20, 2020
View reviewed changes
Copy link
Contributor

@Rikkola Rikkola left a comment

Choose a reason for hiding this comment

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

It looks to me like this follows the convention. Fix itself is good, but it could use some test coverage.

@jomarko
Copy link
Contributor Author

jomarko commented Apr 20, 2020

@Rikkola good point, I have extended the coverage. Thank you.

@dupliaka yes, it is the original behavior. I updated the commit message and description, please let me know if it explains the behavior enough.

@jomarko jomarko requested review from Rikkola and dupliaka April 20, 2020 14:22
@jomarko
Copy link
Contributor Author

jomarko commented Apr 20, 2020

jenkins execute full downstream build

@jomarko
Copy link
Contributor Author

jomarko commented Apr 21, 2020

Failed tests:

org.kie.camel.embedded.camel.component.CamelEndpointWithJaxWrapperCollectionTest.testWorkingSetGlobalTestSessionSetAndGetGlobal
org.kie.camel.embedded.camel.component.JSonBatchExecutionTest.testManualFireAllRules
org.kie.camel.embedded.camel.component.JSonBatchExecutionTest.testInsertElements
org.kie.camel.embedded.camel.component.JSonBatchExecutionTest.testInsertElementsWithReturnObjects
org.kie.camel.embedded.camel.component.XStreamBatchExecutionTest.testInsertElementsWithReturnObjects
org.guvnor.ala.wildfly.executor.tests.WildflyExecutorTest.org.guvnor.ala.wildfly.executor.tests.WildflyExecutorTest
org.guvnor.ala.wildfly.executor.tests.WildflyRuntimeTest.org.guvnor.ala.wildfly.executor.tests.WildflyRuntimeTest

@jomarko
Copy link
Contributor Author

jomarko commented Apr 21, 2020

jenkins execute full downstream build

@jomarko
Copy link
Contributor Author

jomarko commented Apr 22, 2020

jenkins execute full downstream build

@jomarko
Copy link
Contributor Author

jomarko commented Apr 23, 2020

Failed tests:

org.kie.camel.embedded.camel.component.CamelEndpointWithJaxWrapperCollectionTest.testWorkingSetGlobalTestSessionSetAndGetGlobal
org.kie.camel.embedded.camel.component.JSonBatchExecutionTest.testManualFireAllRules
org.kie.camel.embedded.camel.component.JSonBatchExecutionTest.testInsertElements
org.kie.camel.embedded.camel.component.JSonBatchExecutionTest.testInsertElementsWithReturnObjects
org.kie.camel.embedded.camel.component.XStreamBatchExecutionTest.testInsertElementsWithReturnObjects

@jomarko
Copy link
Contributor Author

jomarko commented Apr 23, 2020

jenkins execute full downstream build

@jomarko jomarko mentioned this pull request Apr 23, 2020
Merged
dupliaka
dupliaka approved these changes Apr 23, 2020
View reviewed changes
Copy link
Contributor

@dupliaka dupliaka left a comment

Choose a reason for hiding this comment

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

Thanks for your changes!
Fine for me.

@jomarko
Copy link
Contributor Author

jomarko commented Apr 23, 2020

jenkins execute full downstream build

2 similar comments
@jomarko
Copy link
Contributor Author

jomarko commented Apr 27, 2020

jenkins execute full downstream build

@Ginxo
Copy link
Contributor

Ginxo commented Apr 28, 2020

jenkins execute full downstream build

@Ginxo
Copy link
Contributor

Ginxo commented Apr 28, 2020 •
edited
Loading

relaunched again, job timeout increased

@jomarko
Copy link
Contributor Author

jomarko commented May 11, 2020

jenkins execute full downstream build

2 similar comments
@jomarko
Copy link
Contributor Author

jomarko commented May 13, 2020

jenkins execute full downstream build

@jomarko
Copy link
Contributor Author

jomarko commented May 14, 2020

jenkins execute full downstream build

DROOLS-5249: do not store DateTimeFormatter as free form line
48999c3 
We have two kinds of date boiler plate code snippets. For:
- java.util.Date: 'java.text.SimpleDateFormat sdf'
- java.time.LocalDate: 'java.time.format.DateTimeFormatter dtf'

If user set value either of 'Date' or 'LocalDate' field using 'Literal Value' guided editor action, the given date boiler plate is generated automatically behind the scene. This allows users to put in just value of date, e.g. "01-Jan-2000". User do not have to use parametric constructors, builders, parsers or factory methods. It is done behind the scene using 'sdf' or 'dtf' BoilerPlate.

If users set value either of 'Date' ot 'LocalDate' field using 'Formula' guided editor action, is their responsibility to construct 'Date' or 'LocalDate' instance.

For more details see https://issues.redhat.com/browse/DROOLS-5249
@jomarko
Copy link
Contributor Author

jomarko commented May 15, 2020

jenkins execute full downstream build

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

88.9% 88.9% Coverage
0.0% 0.0% Duplication

@jomarko
Copy link
Contributor Author

jomarko commented May 17, 2020

jenkins execute full downstream build

@jomarko
Copy link
Contributor Author

jomarko commented May 18, 2020

jenkins execute full downstream build

@jomarko
Copy link
Contributor Author

jomarko commented May 20, 2020

No failure but out of memory in downstream build. @Ginxo is this something known?

@Ginxo
Copy link
Contributor

Ginxo commented May 20, 2020

@jomarko yes it is, it is related with archiving build results.
I've created a JIRA for this https://issues.redhat.com/browse/BXMSPROD-793

@jomarko
Copy link
Contributor Author

jomarko commented May 20, 2020

@Ginxo so is it fine if I ask for merge?

@Ginxo
Copy link
Contributor

Ginxo commented May 20, 2020

@jomarko the OOME problem is not related with this particular PR, so if you consider this should be merged please go ahead!

@jomarko
Copy link
Contributor Author

jomarko commented May 21, 2020

@kiegroup/gatekeepers may I ask for merge?

Downstream failed on OOME tracked as mentioned above https://github.com/kiegroup/drools/pull/2859#issuecomment-631310987.

The OOME was logged after kie-wb-distributions execution that is last in our https://github.com/kiegroup/droolsjbpm-build-bootstrap/blob/master/script/repository-list.txt

I grepped console log:

  • ' Failures: ' - 8120 matches
  • ' Failures: 0' - 8120 matches

I think it is fine to merge.

@manstis manstis merged commit fa6ccfe into apache:master May 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dupliaka dupliaka dupliaka approved these changes

@Rikkola Rikkola Rikkola approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jomarko @Ginxo @Rikkola @dupliaka @manstis