Fix for #191 #197

nck-mlcnv · 2023-04-08T12:31:55Z

This pull request also does the following:

Fix the same issue with the countLines method, although it's a quite simple fix for that method.
- For this I've added the getLineEnding method, which returns the used line separator of a file.
Add better tests.

bigerl · 2023-04-11T10:55:13Z

please resolve conflicts

…e-separators

bigerl

Thanks for the PR. I think the code that was used here before is way too over complicated and mostly beyond fixing. It needs more or less a complete rewrite of the respective methods. I suggested some directions in my comments.

iguana.corecontroller/src/main/java/org/aksw/iguana/cc/utils/FileUtils.java

iguana.corecontroller/src/test/java/org/aksw/iguana/cc/utils/FileUtilsTest.java

iguana.corecontroller/src/main/java/org/aksw/iguana/cc/utils/FileUtils.java

iguana.corecontroller/src/test/java/org/aksw/iguana/cc/utils/FileUtilsTest.java

iguana.corecontroller/src/main/java/org/aksw/iguana/cc/utils/IndexedLineReader.java

…tations and use it

...a.corecontroller/src/main/java/org/aksw/iguana/cc/query/source/impl/FileLineQuerySource.java

...econtroller/src/main/java/org/aksw/iguana/cc/query/source/impl/FileSeparatorQuerySource.java

iguana.corecontroller/src/main/java/org/aksw/iguana/cc/utils/FileUtils.java

iguana.corecontroller/src/main/java/org/aksw/iguana/cc/utils/IndexedLineReader.java

bigerl · 2023-04-19T12:01:27Z

Please request an review from @Clueliss when ready.

Clueliss

Hi, so I'm not sure why I'm supposed to review this, because while I do know how to write Java I wouldn't know what constitutes good Java or what is available in the stdlib.

Regarding the exceptions being rethrown or not: I don't know what the goal is here so I can't comment on what's best there.

So in that spirit, this looks fine to me and the tests seem to be passing.

bigerl

One little thing that came to my mind. Thx @Clueliss . I should have further specified, but I think you just did what I amtes you to: have a look for logical errors in the code.

iguana.corecontroller/src/main/java/org/aksw/iguana/cc/utils/FileUtils.java

bigerl

This turns out to be a much bigger change than anticipated. The general structure looks good, now. I've pointed out some bugs, smaller naming improvements and an suggestion for further testing below.

One general comment that is also reflected in some single comments: Please use character-wise reading instead of line-wise reading wherever we read and seek files manually.

iguana.corecontroller/src/main/java/org/aksw/iguana/cc/utils/FileUtils.java

iguana.corecontroller/src/main/java/org/aksw/iguana/cc/utils/IndexedLineReader.java

...econtroller/src/main/java/org/aksw/iguana/cc/query/source/impl/FileSeparatorQuerySource.java

iguana.corecontroller/src/main/java/org/aksw/iguana/cc/utils/IndexedLineReader.java

…ters

bigerl

It's getting into shape. I think we missed one case when parsing the separators. See my comments below.

iguana.corecontroller/src/main/java/org/aksw/iguana/cc/utils/IndexedQueryReader.java

iguana.corecontroller/src/main/java/org/aksw/iguana/cc/utils/FileUtils.java

...troller/src/test/java/org/aksw/iguana/cc/query/source/impl/FileSeparatorQuerySourceTest.java

iguana.corecontroller/src/main/java/org/aksw/iguana/cc/utils/IndexedQueryReader.java

* the separator does not necessarily need to be surrounded by new line chars * Enforce UTF-8 when figuring out the line ending * Implement Knuth-Morris-Pratt on byte-level to index file

nck-mlcnv · 2023-06-22T11:38:55Z

Conflicts have to be resolved and the documentation now needs checked and maybe updated now, because the behaviour of the FileSeparatorQuerySource has been changed.

update documentation
resolve conflicts

…e-separators

bigerl

I had a short look at some of the failing tests. At least some of them seem to be caused by leading/trailing whitespaces/new line characters in tests. The expected strings do not yet have them because they were removed before by the QuerySource.

nck-mlcnv · 2023-06-27T14:28:32Z

So apperently, when the files are uploaded to GitHub they may have different line endings than the original ones. That's why I had to add the behaviour, that the expected values for tests are constructed with the proper line endings.

@ignore

Updated the FileUtilsTest class to create and use temporary files, instead of hard-coded file paths. This change ensures more reliable and portable tests as it no longer relies on specific files being present in the filesystem. Also, removed the '@ignore' annotation to enable the test.

Added a command to ensure that temporary files created during testing are deleted once the process has completed. This is to prevent accumulation of unnecessary test files that could potentially consume excessive memory space.

Changed the "readTest" method in FileUtilsTest.java to use a temporary file, instead of a static file, to improve test isolation and avoid possible interference between tests.

In the `FileUtilsTest` class, the logic for the parameterized test data was refactored. We moved from using hardcoded file paths to dynamically creating temporary files for testing purposes. The test content is now randomly generated UUID strings, allowing for a more diverse dataset for testing. The modified test now checks the hash of the generated string content instead of testing if the hash is simply over 0, ensuring a more accurate analysis.

bigerl

I would remove the textfiles for the tests at some point. See #214 for that. For now, this looks fine and we can move on.

nck-mlcnv added 2 commits April 8, 2023 13:53

fix #191

5398859

simple fix for countLines method

58d71bb

nck-mlcnv linked an issue Apr 8, 2023 that may be closed by this pull request

FileUtils method readLineAt doesn't properly read windows-style line endings #191

Closed

nck-mlcnv requested a review from bigerl April 8, 2023 12:33

Merge branch 'develop' into fix/issue-191/readline-with-different-lin…

23470d2

…e-separators

bigerl requested changes Apr 11, 2023

View reviewed changes

add IndexedLineReader with tests

6798f8f

bigerl reviewed Apr 13, 2023

View reviewed changes

iguana.corecontroller/src/main/java/org/aksw/iguana/cc/utils/IndexedLineReader.java Outdated Show resolved Hide resolved

change the IndexedLineReader to comply with current indexing implemen…

6b8add0

…tations and use it

nck-mlcnv requested a review from bigerl April 14, 2023 10:01

update FileUtils

ae4bae6

bigerl requested changes Apr 14, 2023

View reviewed changes

nck-mlcnv added 4 commits April 18, 2023 21:05

change getLineEnding method in FileUtils class

8bec05d

refactor IndexedLineReader

78ebd18

restructure FileUtilsTest

5848fe2

fix javadoc

ebd623b

nck-mlcnv requested a review from Clueliss April 20, 2023 17:31

Clueliss approved these changes Apr 21, 2023

View reviewed changes

bigerl requested changes Apr 21, 2023

View reviewed changes

iguana.corecontroller/src/main/java/org/aksw/iguana/cc/utils/FileUtils.java Outdated Show resolved Hide resolved

nck-mlcnv added 4 commits April 21, 2023 16:12

fix javadoc again

343becc

correct try-resource-blocks

5b52ab2

add a test for IndexedLineReader

b0ab294

correct try-resource-block for streams too

0077efb

bigerl requested changes May 15, 2023

View reviewed changes

nck-mlcnv added 4 commits May 15, 2023 21:55

make countLines method skip lines that only contain whitespace charac…

70a172e

…ters

add docs for constructor of FileSeparatorQuerySource

4398bdc

rename IndexedLineReader to IndexedQueryReader

41cf274

change try-with-resource instructions

1d8fbf6

nck-mlcnv added 2 commits May 26, 2023 15:49

fix documentation

065e8fe

update constructor and readQuery

1157ce0

nck-mlcnv linked an issue May 26, 2023 that may be closed by this pull request

fix github actions #198

Closed

nck-mlcnv requested a review from bigerl May 26, 2023 14:20

bigerl requested changes Jun 15, 2023

View reviewed changes

nck-mlcnv added 2 commits June 15, 2023 17:16

remove unused methods and other minor changes

74cb9ff

fix indexFile method and add more test cases

4de0a20

nck-mlcnv requested a review from bigerl June 20, 2023 12:50

bigerl requested changes Jun 20, 2023

View reviewed changes

iguana.corecontroller/src/main/java/org/aksw/iguana/cc/utils/IndexedQueryReader.java Show resolved Hide resolved

bigerl mentioned this pull request Jun 20, 2023

Fix/issue 191/rework parsing #211

Merged

Fix/issue 191/rework parsing (#211)

b7b6396

* the separator does not necessarily need to be surrounded by new line chars * Enforce UTF-8 when figuring out the line ending * Implement Knuth-Morris-Pratt on byte-level to index file

nck-mlcnv added 4 commits June 23, 2023 15:10

small change to test

8e1a35d

update documentation

1a86dcf

fix QueryHandlerTest

5783fd5

Merge branch 'develop' into fix/issue-191/readline-with-different-lin…

90404f9

…e-separators

bigerl requested changes Jun 26, 2023

View reviewed changes

nck-mlcnv added 4 commits June 26, 2023 16:43

adjust expected test results to new behaviour of the IndexedQueryReader

41002ac

add tests for getLineEnding

2191621

fix test

cb57354

fix test cases

e82c165

nck-mlcnv requested a review from bigerl June 27, 2023 14:29

bigerl added 4 commits June 28, 2023 09:43

Ensure temporary test files are deleted

18f7d33

Added a command to ensure that temporary files created during testing are deleted once the process has completed. This is to prevent accumulation of unnecessary test files that could potentially consume excessive memory space.

Update FileUtilsTest to use temp file

ff7bbc0

Changed the "readTest" method in FileUtilsTest.java to use a temporary file, instead of a static file, to improve test isolation and avoid possible interference between tests.

bigerl approved these changes Jun 28, 2023

View reviewed changes

bigerl merged commit 922b103 into develop Jun 28, 2023
2 checks passed

bigerl deleted the fix/issue-191/readline-with-different-line-separators branch June 28, 2023 08:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for #191 #197

Fix for #191 #197

nck-mlcnv commented Apr 8, 2023

bigerl commented Apr 11, 2023

bigerl left a comment

bigerl commented Apr 19, 2023

Clueliss left a comment

bigerl left a comment

bigerl left a comment

bigerl left a comment

nck-mlcnv commented Jun 22, 2023 •

edited by bigerl

Loading

bigerl left a comment

nck-mlcnv commented Jun 27, 2023

bigerl left a comment

Fix for #191 #197

Fix for #191 #197

Conversation

nck-mlcnv commented Apr 8, 2023

bigerl commented Apr 11, 2023

bigerl left a comment

Choose a reason for hiding this comment

bigerl commented Apr 19, 2023

Clueliss left a comment

Choose a reason for hiding this comment

bigerl left a comment

Choose a reason for hiding this comment

bigerl left a comment

Choose a reason for hiding this comment

bigerl left a comment

Choose a reason for hiding this comment

nck-mlcnv commented Jun 22, 2023 • edited by bigerl Loading

bigerl left a comment

Choose a reason for hiding this comment

nck-mlcnv commented Jun 27, 2023

bigerl left a comment

Choose a reason for hiding this comment

nck-mlcnv commented Jun 22, 2023 •

edited by bigerl

Loading