PLANNER-1299 #19

danielefiungo · 2018-10-03T08:42:58Z

One mvn build that delegates to the frontend build

.gitignore

.idea/misc.xml

ge0ffrey · 2018-10-04T12:05:34Z

optaweb-tsp-planner/pom.xml

         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
-  <modelVersion>4.0.0</modelVersion>
+    <modelVersion>4.0.0</modelVersion>


@danielefiungo please set your xml indentation to 2 spaces.
We have Intellij/Eclipse code style files here:
https://github.com/kiegroup/droolsjbpm-build-bootstrap/tree/master/ide-configuration

I've applied the coding style.

Warning: the create-react-app project came with eslint configuration that imply a code format.
In our case (tab-size) this don't create problems 2 space match with eslint style, but some other rule can clash with kiegroup standard code format, in this case the "eject" can be used to address the problem with all the related downsides.

I've found a rule conflict,
file : .js rule: indent ESLint CRA 2 spaces vs KIE Conventions 4 spaces

I've edited .eslint.json to adopt KIE Convention but this can became tricky to maintain and adapt.

@danielefiungo KIE Conventions are Java-centric with the default indentation of 4 spaces. Special code style requirements for other languages need to override the default indentation. For example XML language overrides indentation to 2 spaces. I think we need to do the same for JavaScript and TypeScript as the standard indentation for JavaScript code seems to be 2 spaces. Most of the code samples I've seen so far use 2 spaces indentation [1]. What's your opinion, Daniele?

[1] @tiagobento uses 2 space indentation in his proof-of-concept app too: https://github.com/tiagobento/appformer-js/blob/master/packages/core/src/internal/Root.tsx

I agree, using an already widely adopted coding style is a plus.
The only concern is how the linting and formatting of code is done, having two different components ESLint and Intellij/Eclipse.

I think we have at least two options:

Use the .eslint file per project, with eslint plugin for IDEs with disabled Javascript on KIE Conventions (if possible)

Use both trying to obtain the same (if possible)

Isn't only an indent problem:

import React, {Component} from 'react'; // KIE OK // eslint ERROR : A space is required after '{'. (object-curly-spacing) import React, { Component } from 'react';

Even JSX code there is indent issue problem.

Probably when the project will grow we'll have some other mismatch

@danielefiungo .eslintrc.json is the single source of truth. It's based on airbnb style with just a few necessary tweaks. IDEA style need to be adjusted to produce code that has no ESLint errors. That's basically the option 2 that you have proposed. Do you agree with this approach?

Yes, I agree ESLint is the best option.

ge0ffrey · 2018-10-04T12:06:58Z

optaweb-tsp-planner/pom.xml

+        <dependency>
+            <groupId>org.optaplanner</groupId>
+            <artifactId>optaplanner-examples</artifactId>
+            <version>7.12.0-SNAPSHOT</version>


@yurloc Do it similar like optaweb-employee-rostering:
https://github.com/kiegroup/optaweb-employee-rostering/blob/master/pom.xml

Introduce a maven property version.org.optaplanner, use the optaplanner-bom in dependencyManagement, etc Applies to all dependencies.

Yes, but out of scope of this PR (note taken).

ge0ffrey · 2018-10-04T12:08:36Z

optaweb-tsp-planner/pom.xml

+                <dependency>
+                    <groupId>org.optaweb.tsp</groupId>
+                    <artifactId>optaweb-tsp-ui</artifactId>
+                    <version>0.0.1-SNAPSHOT</version>


Use ${project.version} to avoid a pita during MichaelB's releasing (he needs to upgrade the version numbers then).
Better yet, all internal GAV's should be in the depenency management, so having to declare a <version> element to get begin with isn't needed. See optaplanner-examples pom.xml in optaplanner repo.

ge0ffrey · 2018-10-04T12:09:37Z

optaweb-tsp-planner/pom.xml

+    </build>
+    <repositories>
+        <repository>
+            <id>jboss-snapshot</id>


Only the root pom.xml should have a repositories element. That should be exactly the same as the one in optaweb-employee-rostering.

ge0ffrey · 2018-10-04T12:10:27Z

optaweb-tsp-ui/.gitignore

@@ -1,5 +1,6 @@
 # See https://help.github.com/ignore-files/ for more about ignoring files.
-
+### Maven Frontend Plugin ###
+node/


@danielefiungo Can we put the node output under the target directory?

Yes, I'll add the required config

optaweb-tsp-ui/package-lock.json

ge0ffrey · 2018-10-04T12:13:30Z

optaweb-tsp-ui/pom.xml

+
+	<properties>
+		<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
+		<project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>


This pom should extend the root pom. Similar to how optaweb-employee-rostering-shared has a parent optaweb-employee-rostering. When that parent is there, all these properties should be moved there, see the root pom of optaweb-employee-rostering

ge0ffrey · 2018-10-04T12:14:08Z

optaweb-tsp-ui/pom.xml

+			<plugin>
+				<groupId>com.github.eirslett</groupId>
+				<artifactId>frontend-maven-plugin</artifactId>
+				<version>${version.frontend-maven-plugin}</version>


version and most configuration goes in pluginManagement in root pom, see optaweb-employee-rostering root pom

ge0ffrey · 2018-10-04T12:15:24Z

pom.xml

+    <modelVersion>4.0.0</modelVersion>
+
+    <groupId>org.optaweb.tsp</groupId>
+    <artifactId>optaweb-tsp-parent</artifactId>


artfictId of any pom should match the directory name (unless there's a good reason not to do that), let's name this optaweb-traveling-salesman.

ge0ffrey · 2018-10-04T12:16:03Z

pom.xml

+    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+    <modelVersion>4.0.0</modelVersion>
+
+    <groupId>org.optaweb.tsp</groupId>


@yurloc Does this groupId match the java packages? Is it similar in style to how optaweb-employee-rostering does it?

@ge0ffrey Yes, Java classes' packages begin with this groupId, e.g. org.optaweb.tsp.optawebtspplanner.OptawebTspPlannerApplication (packges deeper than org.optaweb.tsp still need to be improved).

ge0ffrey · 2018-10-04T12:20:28Z

@danielefiungo @yurloc I ran through this PR review rather quickly. If the motivation behind any of the comments isn't clear automatically, please let me know and I 'll include a decent motivation behind the reasons for me asking for that particular change.

Generally, we want:

consistency across optaweb-*, optaplanner, kie, jboss.
reduce code duplication (e.g. mvn parents etc)
avoid merge conflicts (e.g. code style and gitignore generated files)

ge0ffrey · 2018-10-04T12:23:19Z

@danielefiungo Good start, please take a look at the comments and let us know when they're fixed.

yurloc · 2018-10-05T14:40:59Z

optaweb-tsp-planner/pom.xml

+        </dependency>
+      </dependencies>
+    </profile>
+  </profiles>


@danielefiungo What is the reason for having the dependency in a profile?

The UI can be deployed on a simple http server and include static resources inside BE can be optionally disabled in CI when UI is deployed separately.

@yurloc Make sense for you?

yurloc · 2018-10-05T15:10:43Z

optaweb-tsp-ui/package-lock.json

+      "resolved": "https://registry.npmjs.org/underscore/-/underscore-1.4.4.tgz",
+      "integrity": "sha1-YaajIBBiKvoHljvzJSA88SI51gQ=",
+      "dev": true
+    },


@danielefiungo Can you explain these changes in package-lock.json? If I run mvn clean install locally, they get removed.

Probably is related to test implementation I've add:
"enzyme": "^3.6.0" and "enzyme-adapter-react-16" without commit package.json to keep it separate.
I'll revert it.

Yeah, makes sense. Thank you!

yurloc · 2018-10-05T15:28:34Z

optaweb-tsp-planner/pom.xml

+              <excludeTransitive>true</excludeTransitive>
+            </configuration>
+          </execution>
+        </executions>


@danielefiungo This has an unpleasant side effect. It unpacks all of this project's dependencies (including Spring Boot starters, H2 database, optaplanner examples) producing a sort of fat JAR. Some resources get overridden (e.g logback.xml). Can you configure the plugin to only unpack the optaweb-tsp-ui dependency?

I've created and removed a review by mistake, anyway ...
the configuration prevent a minor side effect related to the unpack of UI jar that can be addressed with a mvn clean, is minor compared your suggested side effects

yurloc · 2018-10-08T09:25:37Z

package-lock.json

@@ -0,0 +1,551 @@
+{


@danielefiungo I think this file was committed by mistake. There is no package.json in the root directory so there should be no package-lock.json I would say.

Sorry, my bad, probably I had run npm ... on the wrong directory.

ge0ffrey reviewed Oct 4, 2018

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

ge0ffrey reviewed Oct 4, 2018

View reviewed changes

.idea/misc.xml Outdated Show resolved Hide resolved

ge0ffrey reviewed Oct 4, 2018

View reviewed changes

optaweb-tsp-ui/package-lock.json Outdated Show resolved Hide resolved

ge0ffrey reviewed Oct 4, 2018

View reviewed changes

yurloc reviewed Oct 5, 2018

View reviewed changes

yurloc suggested changes Oct 5, 2018

View reviewed changes

yurloc reviewed Oct 8, 2018

View reviewed changes

danielefiungo closed this Oct 25, 2018

danielefiungo force-pushed the master branch from 1b11e93 to efdbe45 Compare October 25, 2018 10:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PLANNER-1299 #19

PLANNER-1299 #19

danielefiungo commented Oct 3, 2018

ge0ffrey Oct 4, 2018

danielefiungo Oct 4, 2018

danielefiungo Oct 5, 2018

yurloc Oct 5, 2018

danielefiungo Oct 5, 2018

yurloc Oct 5, 2018 •

edited

danielefiungo Oct 5, 2018

ge0ffrey Oct 4, 2018 •

edited by yurloc

yurloc Oct 5, 2018

ge0ffrey Oct 4, 2018

ge0ffrey Oct 4, 2018

ge0ffrey Oct 4, 2018

danielefiungo Oct 4, 2018

ge0ffrey Oct 4, 2018

ge0ffrey Oct 4, 2018 •

edited

ge0ffrey Oct 4, 2018

ge0ffrey Oct 4, 2018 •

edited

yurloc Oct 5, 2018

ge0ffrey commented Oct 4, 2018 •

edited

ge0ffrey commented Oct 4, 2018

yurloc Oct 5, 2018

danielefiungo Oct 5, 2018 •

edited

yurloc Oct 5, 2018 •

edited

danielefiungo Oct 5, 2018

yurloc Oct 5, 2018

yurloc Oct 5, 2018

danielefiungo Oct 8, 2018

yurloc Oct 8, 2018

danielefiungo Oct 8, 2018

PLANNER-1299 #19

PLANNER-1299 #19

Conversation

danielefiungo commented Oct 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurloc Oct 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ge0ffrey Oct 4, 2018 • edited by yurloc

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ge0ffrey Oct 4, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ge0ffrey Oct 4, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ge0ffrey commented Oct 4, 2018 • edited

ge0ffrey commented Oct 4, 2018

Choose a reason for hiding this comment

danielefiungo Oct 5, 2018 • edited

Choose a reason for hiding this comment

yurloc Oct 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurloc Oct 5, 2018 •

edited

ge0ffrey Oct 4, 2018 •

edited by yurloc

ge0ffrey Oct 4, 2018 •

edited

ge0ffrey Oct 4, 2018 •

edited

ge0ffrey commented Oct 4, 2018 •

edited

danielefiungo Oct 5, 2018 •

edited

yurloc Oct 5, 2018 •

edited