-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Add fix options for integrity issues #12495
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
base: main
Are you sure you want to change the base?
Conversation
Recording.2025-02-13.mp4There is a problem with selecting messages using the Shift key. One has to click on a message first, as the focus moves to the entry editor. However, when clicking again with the mouse, all the selected entries are lost. |
Recording.2025-02-13.mp4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JUnit tests are failing. In the area "Some checks were not successful", locate "Tests / Unit tests (pull_request)" and click on "Details". This brings you to the test output.
You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide.
Please review |
I have refactored the code a lot and used better practices. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JUnit tests are failing. In the area "Some checks were not successful", locate "Tests / Unit tests (pull_request)" and click on "Details". This brings you to the test output.
You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JUnit tests are failing. In the area "Some checks were not successful", locate "Tests / Unit tests (pull_request)" and click on "Details". This brings you to the test output.
You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JUnit tests are failing. In the area "Some checks were not successful", locate "Tests / Unit tests (pull_request)" and click on "Details". This brings you to the test output.
You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide.
Recording.2025-02-20.mp4The feature is completed. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue seems to be too hard.
No use of formatters at package org.jabref.logic.formatter.bibtexfields
, but some hard-coded fixes.
Will takle longer to get this right.
} | ||
} | ||
|
||
public void maskTitle(IntegrityMessage message) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code seems like AI generated.
I think, I pointed you to org.jabref.logic.cleanup.CleanupJob / org.jabref.logic.cleanup.Formatter
Maybe, we should start on working on a single integrity issue - discuss the archtiecture and then do a second one.
For pages the formatter to use is org.jabref.logic.formatter.bibtexfields.NormalizePagesFormatter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure why it feels AI-generated, it is basic string manipulation, as described in the issue.
Sure, I will use the describe classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following puzzled me very much
updateField(message, StandardField.PAGES, "1-10");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please tell me about the confusion, I am not clear about it.
I created this method because this code was being used in every method for updating fields.
Why I created this method?
Capital letters are not masked using curly brackets {} --> Basic string manipulation
as mentioned in the issue, so to update the field I found this way
Map<Field, String> fields = new HashMap<>();
fields.put(field, newValue);
message.entry().setField(fields);
Then to impove the code reusablity, I created the method updateField
.
From next time I will ask rather then pushing based on assumptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very OK to push - we can then read code. This is much more easy.
I think, this task is really large. (note the "size: big" label).
Thus, it takes time. Will be a back and forth.
The clenaup jobs should do the cleaning, not manually. Look into how the cleanup dialog works.
(I currently cannot do deeper hints. I can try to draft the architecture next week if we cannot manage to move forward efficiently here)
Note that I linked the actions in the screenshot at #12495 (review) - and I hoped, you could find the code starting from that 😅 |
In some of methods like formatPageNumberRange, replaceNonASCIICharacters, ensureValidEdition, as well as writing tests for them, I have used AI due to lack of understanding about the fields, I should rather ask or search on the web. I have planned to ask for improvements about the fixes, which is why I have created fixes for only some issues to limit the usage of AI. Everything else was planned by me, it took me so much iterations from using switch-case in every method to make use of enum, then solving decoupling issues. I used AI just for writing fix methods due to lack of knowledge. |
Hi, it's fine that you learnt what AI does when you use it without full understanding (issue with >95% PRs we see these days) - and thanks for being honest about it. Please be more careful next time, we are all here to answer questions. Also, review AI code very well before using. Even if you didn't have idea about fields, the hardcoding of pages "1-10" in Edit: this was answer to your question in #12495 (comment):
|
public static Optional<IntegrityIssue> fromMessage(IntegrityMessage message) { | ||
return Arrays.stream(IntegrityIssue.values()) | ||
.filter(symbol -> symbol.getText().equals(message.message())) | ||
.findFirst(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method fromMessage does not handle null input for the message parameter, which could lead to a NullPointerException.
Made the combo box display real-time values. Recording.2025-04-01.mp4 |
entryTypeCombo.getSelectionModel().selectFirst(); | ||
} | ||
|
||
public void fix(IntegrityIssue issue, IntegrityMessage message) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix seems a bit slang. "resolveIssue" maybe?
} | ||
|
||
public ObservableList<IntegrityMessage> getMessages() { | ||
return messages; | ||
} | ||
|
||
public void fix(IntegrityIssue issue, IntegrityMessage message) { | ||
// fixes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixes,,,?
@@ -21,7 +20,7 @@ public List<IntegrityMessage> check(BibEntry entry) { | |||
for (Map.Entry<Field, String> field : entry.getFieldMap().entrySet()) { | |||
boolean asciiOnly = CharMatcher.ascii().matchesAllOf(field.getValue()); | |||
if (!asciiOnly) { | |||
results.add(new IntegrityMessage(Localization.lang("Non-ASCII encoded character found"), entry, | |||
results.add(new IntegrityMessage(IntegrityIssue.NON_ASCII_ENCODED_CHARACTER_FOUND.getText(), entry, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Goes a bit beyond the scope of this PR, but you could refactor the IntegrityMessage to accept just the symbol and to resolve the symbol to a string at the caller.
Some changes I haven't commited such as removed the issuesListProperty, because I am working on it. |
} | ||
|
||
public ObservableList<IntegrityMessage> getMessages() { | ||
return messages; | ||
} | ||
|
||
public void resolveIssue(IntegrityIssue issue, IntegrityMessage message) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method 'resolveIssue' is public and should not return null. It should use java.util.Optional to handle potential null values.
Note that your PR will not be reviewed/accepted until you have gone through the mandatory checks in the description and marked each of them them exactly in the format of |
Fixes #11419
This PR introduces a functionality to fix integrity issues either individually or all at once by selecting a type.
Mandatory checks
CHANGELOG.md
described in a way that is understandable for the average user (if change is visible to the user)