search

openzim / zim-tools

Various ZIM command line tools
https://download.openzim.org/release/zim-tools/
GNU General Public License v3.0
133 stars 35 forks source link

--redirect: fix check for number of columns in redirect file #359

Closed haoess closed 1 year ago

kelson42 commented 1 year ago

Not sure exactly what are the rationals behind both bug and fix. But this probably desserves a better automated testing.

mgautierfr commented 1 year ago

Thanks @rgaudin for the investigation.

Here's a valid redirect file [...]

Is it really a valid file ? The regex search for 2 \t (3 fields) but you have 3 \t.

@haoess Please remove the merge commit of your own PR in your own main.

@haoess It would be nice to have a testing of that code. Can you create it ? (It would probably need some refactoring of the code to separate the parsing of the file from adding the redirect)

haoess commented 1 year ago

@haoess Please remove the merge commit of your own PR in your own main.

(Hopefully) done.

@haoess It would be nice to have a testing of that code. Can you create it ?

Sorry, I’m not a C++ dev. Applying some knowledge about how other languages handle captures in RE, using a pattern with three pairs of parens (…) and testing the number of captures (+ 1 for the whole match) against 5 is not supposed to work (= what @rgaudin says).

kelson42 commented 1 year ago

@mgautierfr please don't merge without automated test.

mgautierfr commented 1 year ago

(Hopefully) done.

No, I doesn't. The merge commit was the commit merging the change in your main. By reverting the merge commit, you removing the change. That's why we have 0 file changed.

Anyway, I have create #360 which takes your commit and add the tests. So I'm closing this PR and we will continue on #360. Thanks for your fix anyway.