Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Ignore "Math superscript (^) and subscript (_) characters are not allowed in text mode" error for citation key (#11948) #12391

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Satyamkumarnavneet
Copy link

@Satyamkumarnavneet Satyamkumarnavneet commented Jan 16, 2025

This pull request addresses the issue #11948 of false positive integrity check errors for citation keys containing superscript (^) and subscript (_) characters in text mode. Specifically:

  1. Updated LatexIntegrityChecker#check to:
    • Ignore the validation error TTEM03 for InternalField.KEY_FIELD.
  2. Added test cases to ensure:
    • Citation key fields containing ^ or _ are ignored during validation.
    • Other fields (e.g., TITLE, AUTHOR, JOURNAL, BOOKTITLE, NOTE) are still checked for invalid LaTeX syntax.

Mandatory checks

  • I own the copyright of the code submitted and I licence it under the MIT license
  • Change in CHANGELOG.md described in a way that is understandable for the average user (if change is visible to the user)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • [/] Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • [/] Checked documentation: 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.
Screenshot 2025-01-16 at 6 19 26 PM

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.

In case of issues with the import order, double check that you activated Auto Import.
You can trigger fixing imports by pressing Ctrl+Alt+O to trigger Optimize Imports.

Comment on lines 66 to 67
// Ignore checks for the citation key field
.filter(field -> !(field.getKey().equals(InternalField.KEY_FIELD) && field.getValue().matches(".*[\\^_].*")))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Please take "nore error TTEM03" of the task description serious.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @koppor, Thanks for the feedback! Could you share any suggestions on how to better address the TTEM03 handling?

Comment on lines +308 to +313
Arguments.of(LatexIntegrityChecker.errorMessageFormatHelper(CoreErrorCode.TTEM03), StandardField.TITLE, "Title with ^ and _ characters"),
// Ensure invalid LaTeX syntax with special characters in other fields is detected
Arguments.of(LatexIntegrityChecker.errorMessageFormatHelper(CoreErrorCode.TTEM03), StandardField.AUTHOR, "Author_Name"),
Arguments.of(LatexIntegrityChecker.errorMessageFormatHelper(CoreErrorCode.TTEM03), StandardField.JOURNAL, "Journal_Name"),
Arguments.of(LatexIntegrityChecker.errorMessageFormatHelper(CoreErrorCode.TTEM03), StandardField.BOOKTITLE, "Book^Title"),
Arguments.of(LatexIntegrityChecker.errorMessageFormatHelper(CoreErrorCode.TTEM03), StandardField.NOTE, "Note_with_subscript_and_superscript^_")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, but you did not test for org.jabref.model.entry.field.InternalField#KEY_FIELD, where you changed the behavior.

Copy link
Author

@Satyamkumarnavneet Satyamkumarnavneet Jan 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing that out! I've added a new test case for InternalField.KEY_FIELD, but it's currently failing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume, it is not failing now?

@Satyamkumarnavneet Satyamkumarnavneet marked this pull request as draft January 16, 2025 18:22
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your code currently does not meet JabRef's code guidelines.
We use OpenRewrite to ensure "modern" Java coding practices.
The issues found can be automatically fixed.
Please execute the gradle task rewriteRun, check the results, commit, and push.

You can check the detailed error output by navigating to your pull request, selecting the tab "Checks", section "Tests" (on the left), subsection "OpenRewrite".

@Satyamkumarnavneet Satyamkumarnavneet marked this pull request as ready for review January 17, 2025 12:15
@Satyamkumarnavneet
Copy link
Author

Hi @koppor, I have addressed all feedback and made the necessary changes. Could you please review the updates at your convenience? Thank you!

@@ -66,6 +65,9 @@ public List<IntegrityMessage> check(BibEntry entry) {
.flatMap(LatexIntegrityChecker::getUnescapedAmpersandsWithCount)
// Exclude all DOM building errors as this functionality is not used.
.filter(pair -> !pair.getValue().getErrorCode().getErrorGroup().equals(CoreErrorGroup.TDE))
// Exclude TTEM03 error for citation key field
.filter(pair -> !(pair.getValue().getErrorCode().equals(CoreErrorCode.TTEM03) && pair.getKey().equals(InternalField.KEY_FIELD)))
// Exclude errors present in EXCLUDED_ERRORS set
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this comment? This is really what is in the next line - normally one adds the why on top - or summarizes the next line. Please just remove line 70

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it! Thanks for pointing that out! I’ll remove line 70.

Comment on lines +308 to +313
Arguments.of(LatexIntegrityChecker.errorMessageFormatHelper(CoreErrorCode.TTEM03), StandardField.TITLE, "Title with ^ and _ characters"),
// Ensure invalid LaTeX syntax with special characters in other fields is detected
Arguments.of(LatexIntegrityChecker.errorMessageFormatHelper(CoreErrorCode.TTEM03), StandardField.AUTHOR, "Author_Name"),
Arguments.of(LatexIntegrityChecker.errorMessageFormatHelper(CoreErrorCode.TTEM03), StandardField.JOURNAL, "Journal_Name"),
Arguments.of(LatexIntegrityChecker.errorMessageFormatHelper(CoreErrorCode.TTEM03), StandardField.BOOKTITLE, "Book^Title"),
Arguments.of(LatexIntegrityChecker.errorMessageFormatHelper(CoreErrorCode.TTEM03), StandardField.NOTE, "Note_with_subscript_and_superscript^_")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume, it is not failing now?

@MethodSource("provideCitationKeyInputs")
void citationKeyField(String expectedMessage, Field field, String value) {
entry.setField(field, value);
// Ensure TTEM03 error is not raised for citation key field
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment has to move above Arguments.of("", InternalField.KEY_FIELD, "Corti_2009"), to explain the _ in the inputs. -- In this place, it does not make sense.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's not failing now. It's working for all pre-existing tests as well as the newly added ones.

Screenshot 2025-02-03 at 7 34 58 PM

@Satyamkumarnavneet
Copy link
Author

Satyamkumarnavneet commented Feb 3, 2025

Hi @koppor, I've made the requested changes: removed the comment in LatexIntegrityChecker.java, moved the test comment, and confirmed TTEM03 no longer triggers for citation keys. All tests are passing. Let me know if everything looks good to you and we can proceed with the merge. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants