Quality check escaped ampersand by deybis17 · Pull Request #9758 · JabRef/jabref

deybis17 · 2023-04-12T20:36:46Z

Unscaped ampersands can be found and displayed on the check integrity window
Tests added for escaped ampersand

### Compulsory checks
- [x] Change in `CHANGELOG.md` described in a way that is understandable for the average user (if applicable)
- [x] Tests created for changes (if applicable)
- [x] Manually tested changed features in running JabRef (always required)
- [ ] Screenshots added in PR description (for UI changes)
- [ ] [Checked developer's documentation](https://devdocs.jabref.org/): Is the information available and up to date? If not, I outlined it in this pull request.
- [ ] [Checked documentation](https://docs.jabref.org/): Is the information available and up to date? If not, I created an issue at <https://github.com/JabRef/user-documentation/issues> or, even better, I submitted a pull request to the documentation repository.

Siedlerchr · 2023-04-13T17:17:37Z

Please have a look at the checkstlye issues. Just go to the Files changes tab and reviewdog will show you the offending issues

calixtus · 2023-04-13T20:14:10Z

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.

Yep, Crowdin will overwrite the non english translations on the next update

Thank you for the information. We will make sure to do it the proper way next time.

Siedlerchr

can you please add a changelog something like
" We added a new Integrity check for unscaped ampersands "

Zylence · 2023-04-15T20:10:52Z

can you please add a changelog something like " We added a new Integrity check for unscaped ampersands "

Okay, done :)

Siedlerchr · 2023-04-15T20:17:09Z

Please merge the latest main into it (I just fixed an issue related from another pr) so that the unit tests are green again

…deybis17/jabref into quality-check-escaped-ampersand

koppor

Thank you for the PR! The implementation covered more cases than I thought of. Nice and clean test cases.

Neverthless, I have some nitpicks regarding the tests.

koppor · 2023-04-15T22:58:59Z

+        assertEquals(expected, checker.check(entry));
+    }
+
+    private static Stream<Arguments> provideAcceptedInputs() {


You can rename that method to acceptsAllowedInputs and remove the parameter at @MethodSource

koppor · 2023-04-15T22:59:49Z

+        assertEquals(List.of(new IntegrityMessage(expectedMessage, entry, field)), checker.check(entry));
+    }
+
+    private static Stream<Arguments> provideUnacceptedInputs() {


Similar to above: rename to rejectsDisallowedInputs and change @MethodSource("...") to @MethodSource

koppor · 2023-04-15T23:00:17Z

+        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 \\\\&"),


Small typo:

Suggested change

Arguments.of("Found 1 unescaped '&'", StandardField.AUTHOR, "To many backslashes \\\\&"),

Arguments.of("Found 1 unescaped '&'", StandardField.AUTHOR, "Too many backslashes \\\\&"),

koppor · 2023-04-15T23:01:33Z

+    void entryWithEscapedAndUnescapedAmpersand() {
+        entry.setField(StandardField.TITLE, "Jack \\& Jill & more");
+        assertEquals(List.of(new IntegrityMessage("Found 1 unescaped '&'", entry, StandardField.TITLE)), checker.check(entry));
+    }


Can you integrate that in rejectsDisallowedInputs? I think, there is no additional logic here in this test case.

koppor · 2023-04-15T23:01:43Z

+    @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));
+    }
+}


Can you integrate that in rejectsDisallowedInputs? I think, there is no additional logic here in this test case.

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 · 2023-04-15T23:03:31Z

@Siedlerchr already merged the PR. Maybe, you can do a follow-up PR @deybis17?

Zylence · 2023-04-17T15:19:22Z

@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?

Siedlerchr · 2023-04-17T15:45:54Z

No, it's fine if you include this in your follow up PR!
We have to thank your for your great work! Looking forward to the cleanup action :)

koppor · 2023-04-24T21:05:38Z

This refs #8712.

Zylence added 4 commits March 31, 2023 20:25

Added AmpersandChecker which checks BibEntries for unescaped Ampersands

df849de

Made AmpersandChecker able to handle an arbitrary number of backslashes

5789411

Added UnitTests for AmpersandChecker

94a70f8

Fixed localization

5b110c2

calixtus reviewed Apr 13, 2023

View reviewed changes

Fixed localization and style

83d6aab

Zylence force-pushed the quality-check-escaped-ampersand branch from 593323b to 83d6aab Compare April 14, 2023 15:01

Siedlerchr previously approved these changes Apr 15, 2023

View reviewed changes

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 15, 2023

Updated Changelog

1619e61

Zylence dismissed Siedlerchr’s stale review via 1619e61 April 15, 2023 19:56

Merge branch 'main' into quality-check-escaped-ampersand

5ad2cfe

Siedlerchr marked this pull request as ready for review April 15, 2023 20:16

Zylence and others added 4 commits April 15, 2023 22:24

Merge branch 'main' of https://github.com/deybis17/jabref

2cd91d5

Merged main into quality-check-escaped-ampersand

ccd1e8e

Merge branch 'quality-check-escaped-ampersand' of https://github.com/…

d8c9f8d

…deybis17/jabref into quality-check-escaped-ampersand

Fix reference to koppor repo

38c0350

Siedlerchr approved these changes Apr 15, 2023

View reviewed changes

Siedlerchr merged commit b0ac603 into JabRef:main Apr 15, 2023

Siedlerchr mentioned this pull request Apr 15, 2023

New quality check and cleanup for & JabRef/jabref-koppor#585

Closed

2 tasks

koppor requested changes Apr 15, 2023

View reviewed changes

koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 15, 2023

Zylence mentioned this pull request Sep 21, 2023

Add integrity check for LaTeX special characters #8712

Open

3 tasks

	Arguments.of("Found 1 unescaped '&'", StandardField.AUTHOR, "To many backslashes \\\\&"),
	Arguments.of("Found 1 unescaped '&'", StandardField.AUTHOR, "Too many backslashes \\\\&"),

Uh oh!

Conversation

deybis17 commented Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Siedlerchr commented Apr 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Siedlerchr Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Siedlerchr left a comment

Choose a reason for hiding this comment

Uh oh!

Zylence commented Apr 15, 2023

Uh oh!

Siedlerchr commented Apr 15, 2023

Uh oh!

koppor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koppor commented Apr 15, 2023

Uh oh!

Zylence commented Apr 17, 2023

Uh oh!

Siedlerchr commented Apr 17, 2023

Uh oh!

koppor commented Apr 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

deybis17 commented Apr 12, 2023 •

edited

Loading

Siedlerchr Apr 13, 2023 •

edited

Loading