-
-
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
Be more friendly when using journal alias field #2364
Conversation
Hehe, let us see how political this will get :-) Code-wise the PR looks fine and I give my +1 for merging. The only blocker is that, for some reason, the GUI tests are failing. EDIT: I restartet the build for the push, let's see if it works on a second try. |
You could also create a discussion point at https://github.com/plk/biblatex |
217764a
to
0137c5a
Compare
In case, I want to start a discussion, I'm going to do that on the DANTE e.V. mailing list or same DANTE e.V. event. 😇 |
The GUI tests are also failing on master: https://travis-ci.org/JabRef/jabref/builds/183054056 Refs #1700 |
2f14b88
to
75f38bb
Compare
I don't like this. Despite I'm not using the Biblatex mode I think it is the wrong approach to use the "deprecated" field instead of the intended field only because we are too lazy to implement our fetchers in the right way or because the user is too lazy to convert copied BibTeX entries to biblatex 😉 |
But of course I can see that the new conventions are sometimes inconvenient if users switch between BibTeX and BibLaTeX. As far as I see it, the main problem is that "journal" lands in "deprecated" although it is an alias to a required field. Would it be a solution to show the journal field right below "journaltitle" in "required" if it has a value? Then it would work similar to year+month vs date. |
@matthiasgeiger |
@tobiasdiez Yes, that would be a solution. |
Therefore I put it in quotes ;-) |
7fa3cd9
to
697b05f
Compare
697b05f
to
7e57775
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.
Minor comments but in general LGTM
@@ -345,10 +343,9 @@ public BibEntry toBibEntry(Character keywordDelimiter) { | |||
abstractText.ifPresent(abstractContent -> bibEntry.setField(FieldName.ABSTRACT, abstractContent)); | |||
getDate().ifPresent(date -> bibEntry.setField(FieldName.DATE, date)); | |||
primaryCategory.ifPresent(category -> bibEntry.setField(FieldName.EPRINTCLASS, category)); | |||
journalReferenceText.ifPresent(journal -> bibEntry.setField(FieldName.JOURNALTITLE, journal)); | |||
getPdfUrl().ifPresent(url -> (new TypedBibEntry(bibEntry, BibDatabaseMode.BIBLATEX)) | |||
journalReferenceText.ifPresent(journal -> bibEntry.setField(FieldName.JOURNAL, journal)); |
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 leave journaltitle
here since the returned entry is in biblatex format (see date above)
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.
Done. Although I assume that even with a bibtex database, I get a biblatex entry.
} | ||
|
||
this.setField(FieldName.FILE, newValue); | ||
return Optional.of(new FieldChange(this, FieldName.FILE, oldValue.orElse(""), newValue)); |
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.
return this.setField should be enough
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.
Done.
|
||
import net.sf.jabref.logic.integrity.IntegrityCheck.Checker; | ||
import net.sf.jabref.logic.l10n.Localization; | ||
import net.sf.jabref.model.entry.BibEntry; | ||
import net.sf.jabref.model.entry.EntryConverter; | ||
import net.sf.jabref.model.entry.FieldName; | ||
|
||
public class NoBibtexFieldChecker implements Checker { |
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.
Rename to NoBibLatex ?
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.
Added class comment
This checker checks whether the entry does not contain any field appearing only in BibLaTeX (and not in BibTex)
Thus, the name is IMHO correct.
public void journaltitleIsRecognizedAsBibLaTeXOnlyField() { | ||
BibEntry entry = new BibEntry(); | ||
entry.setField("journaltitle", "test"); | ||
assertNotEquals(Collections.emptyList(), checker.check(entry)); |
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.
This and location should report an error, shouldn't they?
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.
Why? journaltitle
and location
are BibLaTeX only. Thus, everything is allright.
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.
Maybe I just don't understand what the check is doing, but "This checker checks whether the entry does not contain any field appearing only in BibLaTeX" implies for me that I get an error if I use a field (journaltitle) which is only defined in BibLatex and not in Bibtex, right?
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.
It was implemented in #2059.
Yes, you get an error if you use a field (journaltitle) which is only defined in BibLatex and not in Bibtex.
Therefore, I wrote assertNOTequals(emptylist)
. That means, there is an error.
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.
Fixed in c7841a6
7b77d29
to
7f68ccb
Compare
7f68ccb
to
c7841a6
Compare
All comments addressed. Should be good to go now. |
I'd like to copy bibtex entries to biblatex databases and vice versa. When using required fields only, this works perfectly. When in biblatex mode, JabRef does not display the value of
journal
in the required fields, because JabRef demands the fieldjournaltitle
. This is not only confusing for me, but also for other users: #2209.I know that a clean solution is #521. This, however, won't happen this year.
I also know that a bibtex-to-biblatex converter and biblatex-to-bibtex converter is another solution. This, however, forces users to run these when copying and pasting between databases. This could be solved by running these converters silently when copying and pasting. When JabRef is used in parallel to other software (such as Notepad++), this does not help. I cannot force everyone to use JabRef. Thus, simply supporting
journal
in JabRef is the solution for me.The biblatex manual states:
Thus, it is not deprecated, but an alias and should IMHO be considered as full alternative for
journaltitle
.I am aware that this patch makes it hard for users using full biblatex withjournaltitle
instead ofjournal
. I am assuming that not much users are aware of the different modes of JabRef and that they switch back and forth for the same file or for the bibentries.We now show both
journaltitle
andjournal
if these fields exists in as required fields.This whole discussion somehow refs #160
location
).Screenshot: Current JabRef
Screenshot: Updated JabRef
Checklist
gradle localizationUpdate
?