Followup to Issue #3167 #3202
Followup to Issue #3167 #3202
Conversation
Siedlerchr
left a comment
Siedlerchr
left a comment
There was a problem hiding this comment.
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
| - 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) |
| .getFieldMap() | ||
| .entrySet() | ||
| .stream() | ||
| .collect(Collectors.toMap(f -> f.getKey(), f-> BracesCorrector.apply(f.getValue()))); |
There was a problem hiding this comment.
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.
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.
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.
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 |
snisnisniksonah
changed the title
stefan-kolb
added
the
status: ready-for-review
snisnisniksonah
force-pushed
the
fixbraces
branch
from
f24e451 to
a562e1c
Compare
LinusDietz
left a comment
LinusDietz
left a comment
There was a problem hiding this comment.
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.
Siedlerchr
left a comment
Siedlerchr
left a comment
There was a problem hiding this comment.
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 ...
5 participants
Entry editor now adds missing curly braces on closing. #3167
gradle localizationUpdate?