-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Followup to Issue #3167 #3202
Followup to Issue #3167 #3202
Conversation
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.
Some minor changes. And it would be nice if you could add a test for the BracesCorrector to ensure that you captured all edge cases
CHANGELOG.md
Outdated
@@ -35,6 +35,7 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `# | |||
- We fixed an issue where the arrow keys in the search bar did not work as expected [#3081](https://github.com/JabRef/jabref/issues/3081) | |||
- We fixed wrong hotkey being displayed at "automatically file links" in the entry editor | |||
- We fixed an issue where metadata syncing with local and shared database were unstable. It will also fix syncing groups and sub-groups in database. [#2284](https://github.com/JabRef/jabref/issues/2284) | |||
- We fixed and issue where it was possible to leave the entry editor with an imbalance of braces. [#3167](https://github.com/JabRef/jabref/issues/3167) |
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.
and-> an 😉
.getFieldMap() | ||
.entrySet() | ||
.stream() | ||
.collect(Collectors.toMap(f -> f.getKey(), f-> BracesCorrector.apply(f.getValue()))); |
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 think you can use method references for both getKey and getValue here:
https://docs.oracle.com/javase/tutorial/java/javaOO/methodreferences.html
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.
For getKey yes. I couldn't get it to work for getValue tho and I'm not sure if you can use method references if more than one method is called.
|
||
public class BracesCorrector { | ||
|
||
public static String apply(String s) { |
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 use a more descriptive variable name than s
return null; | ||
} else { | ||
String addedBraces = s; | ||
String c = addedBraces.replaceAll("\\\\\\{", "").replaceAll("\\\\\\}", ""); |
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 think it would make sense to extract the regex to a Pattern:
https://stackoverflow.com/questions/1466959/string-replaceall-vs-matcher-replaceall-performance-differences
Hey @snisnisniksonah, is this PR ready for review? If so, please notify us by adding the label and remove the [WIP] tag from the title |
f24e451
to
a562e1c
Compare
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 can live with this solution and we should merge it soon. Please also add a logger event if some braces were added.
Besided that: good job with the tests!
A followup could be to discriminate between the user closing the entry editor (then a dialog should pop up allowing the user to edit the string in question) or jabref is closed then the current method should be called as is.
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.
Looks good from my side! I would merge it in now.
The tests are really useful
* upstream/master: (188 commits) Show file description only if not empty Add file description to gui and fix sync bug (#3210) Don't highlight odd rows in file list editor (#3223) Fix assertion by removing typo (#3220) Update assertion to change of online reference (#3221) Put in null return Reformatted code, renamed method, added try/catch Add tests for removal changes Improve telemetry (#3218) Real test linking real other entry (#3214) Check for explicit group at different location Update java-string-similarity from 0.24 -> 1.0.0 Only remove ExplicitGroups from entries, but not keyword-based groups Localization: French: Translation of a new string (#3212) Fix null return Changed from Path to Optional<Path> Fix #2775: Hyphens in last names are properly parsed (#3209) Followup to Issue #3167 (#3202) Remove unused import in GroupTreeNode Move getEntriesInGroup to GroupTreeNode ...
Entry editor now adds missing curly braces on closing. #3167
gradle localizationUpdate
?