Some corrections that remove platform specific code, causing tests to fail on Windows. #889

alvonellos · 2024-01-27T04:24:55Z

And I noticed some javadoc tags were messed up too, so I cleaned those up as well.

alvonellos · 2024-01-27T04:25:47Z

I tested this thoroughly on local and on wsl linux. Are there any spots I can jump in and take a load off?

ArneBab · 2024-01-27T22:01:32Z

The code changes look good! Thank you very much for contributing!

The only change that would be important before merging is to keep the indentation consistent with the surrounding code (to avoid merge conflicts with other pull-requests we avoid re-indenting everything, so we’re stuck on the current indentation scheme). You might not see the differences in indentation from IntelliJ, they often only show up in the diff-view on github — or on other systems which use a different tab-width.

For stepping in: depending on what you like to do it can be best to start with small things that annoy you (like this pull-request), or to take a task that is likely to have a big impact, i.e. from https://github.com/hyphanet/wiki/wiki/High-Impact-tasks

If you want to see your changes in action quickly, the easiest is to start by improving plugins, because those can be released independently from the full node. Also they would benefit a lot from improvements. See for example some tasks in the section on advanced features: https://github.com/hyphanet/wiki/wiki/High-Impact-tasks#goal-make-advanced-features-easy-to-use

alvonellos · 2024-01-27T22:16:27Z

@ArneBab I made some changes. I think I fixed the spacing? Is there a preferences file that enforces uniform code format?

alvonellos · 2024-01-27T22:17:06Z

The code changes look good! Thank you very much for contributing!

The only change that would be important before merging is to keep the indentation consistent with the surrounding code (to avoid merge conflicts with other pull-requests we avoid re-indenting everything, so we’re stuck on the current indentation scheme). You might not see the differences in indentation from IntelliJ, they often only show up in the diff-view on github — or on other systems which use a different tab-width.

For stepping in: depending on what you like to do it can be best to start with small things that annoy you (like this pull-request), or to take a task that is likely to have a big impact, i.e. from https://github.com/hyphanet/wiki/wiki/High-Impact-tasks

If you want to see your changes in action quickly, the easiest is to start by improving plugins, because those can be released independently from the full node. Also they would benefit a lot from improvements. See for example some tasks in the section on advanced features: https://github.com/hyphanet/wiki/wiki/High-Impact-tasks#goal-make-advanced-features-easy-to-use

I'll take a look at some of the other tasks that are available. :) Thanks for the quick reply @ArneBab

Bombe · 2024-01-28T08:40:17Z

src/freenet/client/filter/M3UFilter.java

@@ -87,15 +87,19 @@ public void readFilter(
                readcount = dis.read(nextbyte);
                continue;
            }
+            if (isCarriageReturn(nextbyte)) {


This looks like it changes the behaviour of the filter but I can’t see that the test is being changed in reaction to this, so either there is no test (in which case, please add a test) or the test doesn’t actually test this behvaiour (in which case, please add a test for the behvaviour).

Bombe · 2024-01-28T08:43:11Z

src/freenet/client/filter/M3UFilter.java

@@ -164,7 +168,7 @@ public void readFilter(
                            }
                            // write the newline if we're not at EOF
                            if (readcount != -1){
-                                dos.write(nextbyte);
+                                dos.write(System.lineSeparator().getBytes(StandardCharsets.UTF_8));


This change makes the filter’s behaviour dependent on the OS Fred is being run on. Is that actually what we want? In my opinion our filters should behave the same everywhere, especially as the output of the filter is supposed to be usable on every OS. Now, we of course do not know here what OS the output of the filter is being used on so really any choice we make here is an arbitrary one, which is a rather long-winded way of saying that we shouldn’t actually change anything here. 🙂

I think the likelyhood that we’ll use the file on Windows if the filter runs on Windows is pretty high. And there are still editors on Windows that display a file with \n as line separator as just one long line.

I don’t think any audio player makes that difference, though, so this is rather about people reading the code, not about functionality.

True, people using notepad.exe would probably prefer Windows-style line endings… 😁

But, this would also expose that the node is running on a Windows system. I’m not sure how much of an issue that is, though… are we worrying about that?

Bombe · 2024-01-28T08:43:41Z

src/freenet/support/HTMLNode.java

@@ -418,7 +418,7 @@ public StringBuilder generate(StringBuilder tagBuffer, int indentDepth ) {
 			}
 		} else {
 			if (newlineOpen(name)) {
-				tagBuffer.append('\n');
+				tagBuffer.append(System.lineSeparator());


Here it’s basically the same reasoning: we can’t actually know that the HTML file is being parsed on the same OS that we use to create the HTML.

Bombe · 2024-01-28T08:45:25Z

test/freenet/clients/http/utils/PebbleUtilsTest.java

@@ -15,7 +15,7 @@ public class PebbleUtilsTest {
 	@Test
 	public void addChildAddsCorrectlyEvaluatedSimpleTemplateToHtmlNode() throws IOException {
 		PebbleUtils.addChild(emptyParentNode, "pebble-utils-test-simple", model, null);
-		assertThat(emptyParentNode.generate(), equalTo("Test!\n"));
+		assertThat(emptyParentNode.generate(), equalTo("Test!" + System.lineSeparator()));


Now, this is actually interesting. What we do want here is to test that the template substitution works correctly, which means that we very much do want to avoid using system-dependent behaviour here; however, that also means that we need to make sure that the source of the template contains exactly the line endings that we expect in the test. I guess the real fix here is to make sure that Git doesn’t change the line endings of the file.

I hadn’t thought of git changing the line endings in the test files! 🤦

That won’t happen in an unpacked tarball, so we’d get test failures there.

@alvonellos can you check whether the linesep can also be fixed by adding a .gitattributes file for our expected test output similar to this? https://shzhangji.com/blog/2022/08/31/configure-git-line-endings-across-oses/

I'm looking into this.

alvonellos · 2024-01-31T21:07:29Z

Arne, I’ll look at this and get back to you. 😊 Best, * Alex From: Arne Babenhauserheide ***@***.***> Sent: Sunday, January 28, 2024 4:03 AM To: hyphanet/fred ***@***.***> Cc: Alexander Alvonellos ***@***.***>; Mention ***@***.***> Subject: Re: [hyphanet/fred] Some corrections that remove platform specific code, causing tests to fail on Windows. (PR #889) @ArneBab commented on this pull request.

_____ In test/freenet/clients/http/utils/PebbleUtilsTest.java <#889 (comment)> :

@@ -15,7 +15,7 @@ public class PebbleUtilsTest {

@test public void addChildAddsCorrectlyEvaluatedSimpleTemplateToHtmlNode() throws IOException { PebbleUtils.addChild(emptyParentNode, "pebble-utils-test-simple", model, null); - assertThat(emptyParentNode.generate(), equalTo("Test!\n")); + assertThat(emptyParentNode.generate(), equalTo("Test!" + System.lineSeparator())); I hadn’t thought of git changing the line endings in the test files! 🤦 That won’t happen in an unpacked tarball, so we’d get test failures there. @alvonellos <https://github.com/alvonellos> can you check whether the linesep can also be fixed by adding a .gitattributes file for our expected test output similar to this? https://shzhangji.com/blog/2022/08/31/configure-git-line-endings-across-oses/ — Reply to this email directly, view it on GitHub <#889 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAPC2JGRRDTMRO47OJHQRN3YQYH4TAVCNFSM6AAAAABCNCA37KVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQNBXGQ2DOMZTGE> . You are receiving this because you were mentioned. <https://github.com/notifications/beacon/AAPC2JDPSS6WWPNDLCOMZZTYQYH4TA5CNFSM6AAAAABCNCA37KWGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTTODXHSG.gif> Message ID: ***@***.*** ***@***.***> >

alvonellos · 2024-02-01T21:59:36Z

I will undo my changes here for the time being.

alvonellos · 2024-02-01T22:21:57Z

I have made a new branch and pushed the git attributes change. There's still something funky going on with the M3U Filters. I'm looking into this.

ArneBab · 2024-02-01T23:16:26Z

Thank you! I know that it often feels strange to throw away code, but at least for me, finding the cleaner fix more than makes up for it. ❤️

alvonellos closed this Jan 27, 2024

alvonellos reopened this Jan 27, 2024

Bombe requested changes Jan 28, 2024

View reviewed changes

alvonellos closed this Feb 1, 2024

alvonellos force-pushed the next branch from 2ba4e84 to ba8249c Compare February 1, 2024 21:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some corrections that remove platform specific code, causing tests to fail on Windows. #889

Some corrections that remove platform specific code, causing tests to fail on Windows. #889

alvonellos commented Jan 27, 2024

alvonellos commented Jan 27, 2024 •

edited

Loading

ArneBab commented Jan 27, 2024

alvonellos commented Jan 27, 2024

alvonellos commented Jan 27, 2024

Bombe Jan 28, 2024

Bombe Jan 28, 2024

ArneBab Jan 28, 2024

Bombe Jan 28, 2024

Bombe Jan 28, 2024

Bombe Jan 28, 2024

ArneBab Jan 28, 2024

alvonellos Jan 31, 2024

alvonellos commented Jan 31, 2024 via email

alvonellos commented Feb 1, 2024

alvonellos commented Feb 1, 2024

ArneBab commented Feb 1, 2024

Some corrections that remove platform specific code, causing tests to fail on Windows. #889

Some corrections that remove platform specific code, causing tests to fail on Windows. #889

Conversation

alvonellos commented Jan 27, 2024

alvonellos commented Jan 27, 2024 • edited Loading

ArneBab commented Jan 27, 2024

alvonellos commented Jan 27, 2024

alvonellos commented Jan 27, 2024

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

alvonellos commented Jan 31, 2024 via email

alvonellos commented Feb 1, 2024

alvonellos commented Feb 1, 2024

ArneBab commented Feb 1, 2024

alvonellos commented Jan 27, 2024 •

edited

Loading