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

Followup to Issue #3167 #3202

Merged
merged 6 commits into from
Sep 12, 2017
Merged

Followup to Issue #3167 #3202

merged 6 commits into from
Sep 12, 2017

Conversation

snisnisniksonah
Copy link
Contributor

@snisnisniksonah snisnisniksonah commented Sep 7, 2017

Entry editor now adds missing curly braces on closing. #3167

  • Change in CHANGELOG.md described
  • Tests created for changes
  • Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)
  • If you changed the localization: Did you run gradle localizationUpdate?

@snisnisniksonah snisnisniksonah changed the title Followup to Issue #3167 [WIP]Followup to Issue #3167 Sep 7, 2017
@koppor koppor changed the title [WIP]Followup to Issue #3167 [WIP] Followup to Issue #3167 Sep 7, 2017
@koppor koppor added this to the v4.0 milestone Sep 7, 2017
Copy link
Member

@Siedlerchr Siedlerchr left a 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)
Copy link
Member

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())));
Copy link
Member

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

Copy link
Contributor Author

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) {
Copy link
Member

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("\\\\\\}", "");
Copy link
Member

Choose a reason for hiding this comment

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

@LinusDietz
Copy link
Member

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 snisnisniksonah changed the title [WIP] Followup to Issue #3167 Followup to Issue #3167 Sep 11, 2017
@stefan-kolb stefan-kolb added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Sep 12, 2017
Copy link
Member

@LinusDietz LinusDietz left a 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.

Copy link
Member

@Siedlerchr Siedlerchr left a 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

@Siedlerchr Siedlerchr merged commit 58fec29 into JabRef:master Sep 12, 2017
Siedlerchr added a commit that referenced this pull request Sep 13, 2017
* upstream/master:
  Localization: French: Translation of a new string (#3212)
  Fix #2775: Hyphens in last names are properly parsed (#3209)
  Followup to Issue #3167 (#3202)
  Update mockito-core from 2.9.0 -> 2.10.0
@koppor koppor deleted the fixbraces branch September 14, 2017 09:09
Siedlerchr added a commit that referenced this pull request Sep 23, 2017
* 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
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants