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.

Sign up for GitHub

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

Jump to bottom

Fix #3359: Automatically remove colon and apostrophe from key pattern #3506

Merged
merged 9 commits into from
Jan 2, 2018
Merged

Fix #3359: Automatically remove colon and apostrophe from key pattern #3506

merged 9 commits into from
Jan 2, 2018

Conversation

tobiasdiez
Copy link
Member

Fixes #3359. Colons and apostrophes are now removed from the generated key pattern.
I also refactored the key generator, mainly converting the static methods to instance methods.

  • 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?

@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 9, 2017
lenhard
lenhard reviewed Dec 10, 2017
View reviewed changes
Copy link
Member

@lenhard lenhard left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks like a nice refactoring overall, which makes the key generator much more pleasant to use in code, I like that. I have nothing to criticize code-wise. But you still seem to have some checkstyle issues, if I am not mistaken.

I have tested this locally and key generation works and leaves out the characters we want to avoid. I am a still a little doubtful since there are so many different use cases this PR touches due to the refactoring. I have just tested the most straight-forward one and I hope that no bugs were introduced for others (e.g. Open Office). But I trust you on this and at least there are plenty of tests.

@tobiasdiez
Copy link
Member Author

@lenhard I manually tested most use cases, where the code changes (except OpenOffice since I don't use it). What are our current plans for the 4.1 release? When do we want to release? If there is still some time before the release I would prefer if this PR is merged before the release.

@Siedlerchr
Copy link
Member

You may want to add the percent sign and the ampersand (&), which is problematic as it creates a problem with biber/biblatex

Siedlerchr
Siedlerchr reviewed Dec 10, 2017
View reviewed changes
src/main/java/org/jabref/logic/bibtexkeypattern/BibtexKeyGenerator.java Outdated
StringBuilder newKey = new StringBuilder();
for (int i = 0; i < key.length(); i++) {
char c = key.charAt(i);
if (!Character.isWhitespace(c) && ("{}(),\\\"#~^':`".indexOf(c) == -1)) {
Copy link
Member

Choose a reason for hiding this comment

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

You should reeally extract this to a constant, same with th e one abvoe

@koppor koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 22, 2017
@koppor koppor changed the title Fix #3359: Automatically remove colon and apostrophe from key pattern [WIP] Fix #3359: Automatically remove colon and apostrophe from key pattern Dec 22, 2017
@tobiasdiez tobiasdiez added this to the v4.2 milestone Dec 23, 2017
@tobiasdiez tobiasdiez changed the title [WIP] Fix #3359: Automatically remove colon and apostrophe from key pattern Fix #3359: Automatically remove colon and apostrophe from key pattern Dec 23, 2017
@tobiasdiez
Copy link
Member Author

Should this still go into 4.1?

@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 23, 2017
@lenhard
Copy link
Member

lenhard commented Jan 2, 2018

@tobiasdiez When you fix the checkstyle issues and conflicts then this PR can be merged.

@tobiasdiez tobiasdiez merged commit c438554 into master Jan 2, 2018
@tobiasdiez tobiasdiez deleted the fix3359 branch January 2, 2018 20:19
Siedlerchr added a commit that referenced this pull request Jan 2, 2018
@Siedlerchr
Merge remote-tracking branch 'upstream/master' into selectFilesDlg
d3daaa6 
* upstream/master:
  Add oaDOI fulltext fetcher (#3581)
  Refactor export code to fix #3576 (#3578)
  Fix #3359: Automatically remove colon and apostrophe from key pattern (#3506)
@klamkiew klamkiew mentioned this pull request May 4, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lenhard lenhard lenhard left review comments

@Siedlerchr Siedlerchr Siedlerchr approved these changes

Assignees
No one assigned
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Milestone
v4.2
Development

Successfully merging this pull request may close these issues.
4 participants
@tobiasdiez @Siedlerchr @lenhard @koppor