Quality check escaped ampersand #9758
Conversation
|
Please have a look at the checkstlye issues. Just go to the Files changes tab and reviewdog will show you the offending issues |
There was a problem hiding this comment.
Just a note: Although this should work both ways, translations should normally only be done with crowdin. Unified and tested common workflows lead to less errors.
There was a problem hiding this comment.
Yep, Crowdin will overwrite the non english translations on the next update
There was a problem hiding this comment.
Thank you for the information. We will make sure to do it the proper way next time.
Zylence
force-pushed
the
quality-check-escaped-ampersand
branch
from
593323b to
83d6aab
Compare
Siedlerchr
added
the
status: ready-for-review
Okay, done :) |
|
Please merge the latest main into it (I just fixed an issue related from another pr) so that the unit tests are green again |
| assertEquals(expected, checker.check(entry)); | ||
| } | ||
|
|
||
| private static Stream<Arguments> provideAcceptedInputs() { |
There was a problem hiding this comment.
You can rename that method to acceptsAllowedInputs and remove the parameter at @MethodSource
| assertEquals(List.of(new IntegrityMessage(expectedMessage, entry, field)), checker.check(entry)); | ||
| } | ||
|
|
||
| private static Stream<Arguments> provideUnacceptedInputs() { |
There was a problem hiding this comment.
Similar to above: rename to rejectsDisallowedInputs and change @MethodSource("...") to @MethodSource
| return Stream.of( | ||
| Arguments.of("Found 1 unescaped '&'", StandardField.SUBTITLE, "A single &"), | ||
| Arguments.of("Found 2 unescaped '&'", StandardField.ABSTRACT, "Multiple \\\\& not properly & escaped"), | ||
| Arguments.of("Found 1 unescaped '&'", StandardField.AUTHOR, "To many backslashes \\\\&"), |
There was a problem hiding this comment.
Small typo:
| Arguments.of("Found 1 unescaped '&'", StandardField.AUTHOR, "To many backslashes \\\\&"), | |
| Arguments.of("Found 1 unescaped '&'", StandardField.AUTHOR, "Too many backslashes \\\\&"), |
| void entryWithEscapedAndUnescapedAmpersand() { | ||
| entry.setField(StandardField.TITLE, "Jack \\& Jill & more"); | ||
| assertEquals(List.of(new IntegrityMessage("Found 1 unescaped '&'", entry, StandardField.TITLE)), checker.check(entry)); | ||
| } |
There was a problem hiding this comment.
Can you integrate that in rejectsDisallowedInputs? I think, there is no additional logic here in this test case.
| @Test | ||
| void entryWithMultipleEscapedAndUnescapedAmpersands() { | ||
| entry.setField(StandardField.AFTERWORD, "May the force be with you & live long \\\\& prosper \\& to infinity \\\\\\& beyond & assemble \\\\\\\\& excelsior!"); | ||
| assertEquals(List.of(new IntegrityMessage("Found 4 unescaped '&'", entry, StandardField.AFTERWORD)), checker.check(entry)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you integrate that in rejectsDisallowedInputs? I think, there is no additional logic here in this test case.
There was a problem hiding this comment.
Sure, we can do that. The reasoning behind having it separated was that it basically combined the test cases from before, containing both escaped and unescaped ampersands (in the parametrized tests there was no mixing of the two).
koppor
removed
the
status: ready-for-review
|
@Siedlerchr already merged the PR. Maybe, you can do a follow-up PR @deybis17? |
Thank you for the feedback, and thanks @Siedlerchr for merging our PR! :-) We are happy to implement the requested changes, may we include them in the next PR (which is going to address the related cleanup action for ampersands) or should we make the changes in a separate PR? |
|
No, it's fine if you include this in your follow up PR! |
|
This refs #8712. |
Fixes for #8948
Fixes JabRef#585
Compulsory checks