Fix #441 allowing to build project with Windows #904

lujop · 2021-04-24T21:50:27Z

Thanks for contributing.

Tick to sign-off your agreement to the Developer Certificate of Origin (DCO) 1.1

Description

Fixes issue #441 allowing to build project with Windows and with systems that don't has en-US as default locale

Changes made:

Changing hardcoded line separator for `System.lineSeparator()'. Except in HTML writer because Jsoup seems to use always Unix line separator.
Putting all surefire args in one line because two didn't work and system locale was used.
Changed mocked URLs because Inet URLs were used as if they were valid local filesystem urls.
Disabled BOM test. I think that test being failing is correct, at least in systems that don't use UTF-8. I haven't seen any management of BOM in the tablesaw code, and univocity seems to manage BOM but only when a File is passed and not a Reader as tablesaw uses.

benmccann · 2021-04-26T05:19:24Z

core/src/test/java/tech/tablesaw/analytic/AnalyticQueryTest.java

-            + "FROM myTable\n"
-            + "Window w1 AS (\n"
+        "SELECT"
+            + System.lineSeparator()


this seems a little hard to read to me. Can we put each System.lineSeparator() on the end of the line instead of putting it on a new line?

Yes, I don't like also. But it was done by the formatter plugin that is configurated in maven.
In fact, it separated a lot of lines from the way I initially leave them.
Is it used?

it wouldn't hurt to use a static import so you could just write lineSeparator(). Even that is too verbose, but you can't fight Java names.

You could make it even shorter by assigning it to a constant at the top of the file like NEW_LINE

One day we'll have Java 13's triple quotes and this will all be cleaner, but it's probably too early for that since Java 11 is the last LTS unless maybe we can just run the tests on Java 13, but compile with Java 11

Yes I can use static import to reduce size, but about the new line I don't like it too.
But is what made the formatter plugin. I suppose that it sees a string concatenation that is two big and wants every concatenation in its new line.

What do you want me to do?
Only static import?
Reformat by hand trying to use fewer lines and ignoring formatter plugin? In that case are there any formatting rules that I must follow?

You shouldn't format by hand. I'm not sure how the other options would look, but they're worth experimenting with

Ok, then I should do just use a static import on all line separators? Any other thing it's needed to be changed?

I would say we should try it and see how it looks

At the end, I use a LINE_END constant that I've seen that was used in some places.

benmccann · 2021-04-27T17:59:56Z

core/src/test/java/tech/tablesaw/io/DataFrameReaderTest.java

@@ -50,8 +50,7 @@ public void csv() throws IOException {
  @Test
  public void readUrlWithExtension() throws Exception {
    URL url =
-        mockUrlHelper(
-            "http://something.other.com/file.csv", ImmutableList.of("region", "canada", "us"));
+        mockUrlHelper("something.other.com/file.csv", ImmutableList.of("region", "canada", "us"));


It looks weird to me that http:// is missing. I didn't understand the explanation for why it had to be changed. Is there a fix we can make to mockUrlHelper or something that will allow http:// to be used?

Changed the mockUrl to be able to use urls

core/src/test/java/tech/tablesaw/io/csv/UnicodeBomHandlingTest.java

benmccann · 2021-04-28T21:52:18Z

jsplot/src/test/java/tech/tablesaw/components/LayoutTest.java

-              + //
-              "};\n",
+          "var layout = {"
+              + lineSeparator()


this one should be LINE_END to match the others and then you no longer need the static import

benmccann · 2021-04-28T21:53:22Z

can you rebase this PR? thanks!

Change System.lineSeparator() for a constant to be less verbose Change mockUrl to be able to use URLs

benmccann · 2021-04-29T16:08:28Z

Can you try adding Windows to the CI so that we avoid regressing? It should just be changing the line below:

tablesaw/.github/workflows/ci.yml

Line 19 in 56508ba

# os: [windows-latest, macOS-latest, ubuntu-latest]

lujop · 2021-04-29T16:53:48Z

Can you try adding Windows to the CI so that we avoid regressing? It should just be changing the line below:

tablesaw/.github/workflows/ci.yml

Line 19 in 56508ba

# os: [windows-latest, macOS-latest, ubuntu-latest]

Done

benmccann · 2021-04-29T17:05:53Z

.github/workflows/ci.yml

@@ -17,7 +17,7 @@ jobs:
        java-version: [1.8, 11, 15]
        # Tests seem to be failing on Windows


can you remove this line and the next line?

OMG, I didn't see it.
Done

lujop · 2021-04-29T17:29:22Z

I've seen that build on windows crashes on JAva 15,
Can I change for 16 that is the lastest now and retry?

benmccann · 2021-04-29T17:31:36Z

Hmm, that's weird. But sure, go ahead and try changing 15 to 16

benmccann reviewed Apr 26, 2021

View reviewed changes

lujop mentioned this pull request Apr 26, 2021

Fixes #822 and #815 providing more extensive columntype options for readers #909

Merged

1 task

benmccann reviewed Apr 27, 2021

View reviewed changes

core/src/test/java/tech/tablesaw/io/csv/UnicodeBomHandlingTest.java Show resolved Hide resolved

lujop requested a review from benmccann April 28, 2021 19:42

benmccann reviewed Apr 28, 2021

View reviewed changes

lujop added 2 commits April 29, 2021 09:18

Fix jtablesaw#441 allowing to build project with Windows

61ca25a

Fix minor PR suggestions

4d47a22

Change System.lineSeparator() for a constant to be less verbose Change mockUrl to be able to use URLs

lujop force-pushed the fixwindowsbuild branch from 1adef31 to 4d47a22 Compare April 29, 2021 07:18

lujop requested a review from benmccann April 29, 2021 16:01

Add Windows to CI

543f3c9

benmccann reviewed Apr 29, 2021

View reviewed changes

Remove comment about windows build failing

110ee1b

lujop requested a review from benmccann April 29, 2021 17:27

lujop added 2 commits April 29, 2021 19:36

Upgrade last java version to use for CI to 16

2e681ea

Update jacoco version because previous crashes on windows with Java>15

eaf5d1f

benmccann merged commit b6c138a into jtablesaw:master Apr 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #441 allowing to build project with Windows #904

Fix #441 allowing to build project with Windows #904

lujop commented Apr 24, 2021

benmccann Apr 26, 2021

lujop Apr 26, 2021

lwhite1 Apr 27, 2021

benmccann Apr 27, 2021

lujop Apr 27, 2021

benmccann Apr 27, 2021

lujop Apr 27, 2021

benmccann Apr 27, 2021

lujop Apr 28, 2021

benmccann Apr 27, 2021

lujop Apr 28, 2021

benmccann Apr 28, 2021

benmccann commented Apr 28, 2021

benmccann commented Apr 29, 2021

lujop commented Apr 29, 2021

benmccann Apr 29, 2021

lujop Apr 29, 2021

lujop commented Apr 29, 2021

benmccann commented Apr 29, 2021

		@@ -17,7 +17,7 @@ jobs:
		java-version: [1.8, 11, 15]
		# Tests seem to be failing on Windows

Fix #441 allowing to build project with Windows #904

Fix #441 allowing to build project with Windows #904

Conversation

lujop commented Apr 24, 2021

Description

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

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

benmccann commented Apr 28, 2021

benmccann commented Apr 29, 2021

lujop commented Apr 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lujop commented Apr 29, 2021

benmccann commented Apr 29, 2021