-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
FIx chinese author bibtex key generation #4226
Conversation
fix-for-issue-4169
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.
In general looks good, if you change the optional check with empty and a second Dev gives his okay we can merge it.
Thanks a lot for your contribution!
@@ -570,6 +570,32 @@ public static String firstAuthor(String authorField) { | |||
if (authorList.isEmpty()) { | |||
return ""; | |||
} | |||
if (authorList.getAuthor(0).getVon().toString().equals("Optional.empty")) { |
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.
Much easier, Optionals have a method names isPresent https://docs.oracle.com/javase/8/docs/api/java/util/Optional.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.
Thanks for your suggestion! We will change it better.
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.
Thanks for you contribution. In general the code looks good and it's nice that you already added tests. However, I think, the fix is in the wrong location (see my comment below for more details about this).
|| (ub == Character.UnicodeBlock.CJK_SYMBOLS_AND_PUNCTUATION) | ||
|| (ub == Character.UnicodeBlock.HALFWIDTH_AND_FULLWIDTH_FORMS)) { | ||
return authorList.getAuthor(0).getVon().orElse(""); | ||
} |
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.
If I understand this code correctly, the author parsing is wrong for Chinese names and you correct it here for the bibtex generator. However, we also rely on the author parsing in different contexts (like the display of the author in the maintable). It would be nice if you could move this fix to the AuthorListParser
class so that it also applies to these other instances. Please add corresponding tests in AuthorListTest
(where you should orient yourself on the format of the test parseNameWithHyphenInFirstName
- some of the other tests in this class are not so nice). You should keep the tests for the bibtex key generator.
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.
Well, I understand your meaning. At first, I did want to change the code in the way you suggested. But then I found that the key of author filed containing Chinese name is totally different from the situation of English name. In AuthorListParser.class, there are four situations for the split of English name(two based on no commas, one based on one commas,one based on two or more commas), while there are only two fixed situations and other instances don't apply to Chinese name. Also it's easier to fix the bug in bibtexkeypattern.class while it's complex and not neccessary to move this fix to the AuthorListParser.class.
This reverts commit 438de43.
change the optional check with empty
If I remeber correctly this issue was now resolved with the other PR #4248 |
-We fixed an issue where the default bibtexkey's generation is incorrect for Chinese authors.
For example:
suppose that the author field such as 王, 阳 and 张, 家发 and 胡, 喜军 and 李, 保明, the year is 1000 and the default bibtexkey's generation is [author][year]
the original output is:
1000 (it is obviously incorrect)
we add some code to address it
now the output is:
王1000 (which is specified as user guide)
well, we also add some test code and passed these tests successfully
Fixes #4169