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

Change EntryType to String in BibEntry #605

Merged
merged 3 commits into from
Jan 26, 2016

Conversation

tobiasdiez
Copy link
Member

This PR tries to resolve the issues discussed in #337 and #495.

Right now the entry type is converted from a string representation (in the bibtex file) to an EntryType by the parser. The problem with this approach is that the parser has to know about the bibtex vs biblatex mode. The idea was to just store the string in the BibEntry and only get the corresponding EntryType whenever it is really necessary.

@koppor
Copy link
Member

koppor commented Jan 1, 2016

Oh, that change surely was much effort. Unfortunately, I find the old way more fitting even though the bibtex / biblatex mode switching was difficult. Why not using EntryType, but only as "proxy" to the real type dependent on the bibtex/biblatex mode? (I currently don't know the GoF pattern name for that 🙈)

All entries should have a (non-abstract) EntryType and there should be (abstract) BibtexEntryType and BiblatexEntryType. All of them should implement IEntryType offering hasAllRequiredFieldsetc. EntryTypeand *EntryType should not be in a inheritance relation. EntryType should proxy to the appropriate Bib(la)texType depending on the mode.

@tobiasdiez
Copy link
Member Author

Thanks Olly for your feedback.

Let me elaborate a little about why I've chosen to store the entry type as a String.
The type of the entry is per se a state-less information. @article{} signifies an article in the bib file independent on any preferences in JabRef. I think it it thus a good idea to also have a state-independent representation of the same information in JabRef. In other words, I prefer a system where the type is stored in the BibEntry class independent on anything else (much like the author field is also stored as a string and not as a AuthorList object). Then in a latter step this data can be enriched by the preference-dependent information about optional and required fields. This reflects more or less the impression I got from #495 that we came to the conclusion to see the BibEntry class only as a data object without any logic.

Now you can think about implementing such a system in Java. The easiest way and the one I have chosen in this PR is to store the type as a String variable. One can also store the type information as an enum (as I have suggested in #501), but this feels over-engineered (especially since there is no code checking whether a entry is an article or a book).
The second step is to augment this information by optional/required fields. I have chosen a constructor-based approach, but maybe your idea about using the proxy pattern is better. I have to think about it more.

@koppor
Copy link
Member

koppor commented Jan 1, 2016

Regarding String vs AuthorList: I would have preferred that BibEntry uses AuthorList. Possibly calculated on demand and not during creation of the object. Similar for the files field.

Unsure whether I'm wrong. I would pack as much "interpreted" data in BibEntry and get rid of Strings as much as possible... Maybe, we then reimplement an in-memory relational database system. For instance, if we store the "crossref" as reference and not as string. What if the referenced entry is deleted and then recreated etc.

@tobiasdiez
Copy link
Member Author

Ok, as requested here a short summary in bullet list form.

Status quo: Parser converts the string "article" to a BibType class instance and saves this in the entry.

  • (--) Makes changing from BibTex to BibLatex mode hard as the type stored in every entry has to be changed.
  • (-) Adds additional dependency in the parser (BibTypes class).
  • (-) The parser has to know whether we are in BibLatexMode or in BibTexMode. However, a priori this information is not necessary to parse a bib file.
  • (+) Does not store the type information as a String

PR: Parser stores the type information as a plain string in the entry. This string will be converted to BibType class when needed

  • (+) Changing from BibTex to BibLatex mode is easier since the required/optional fields are used only in a few places.
  • (+) Makes dependencies clearer (which class needs to know the BibType or BibTex/BibLatex mode).
  • (+) Parser does not depend on BibType class and on BibLatexMode.
  • (0-) Stores the type information as a string. However, this is in my opinion no real caveat. The previous code also contains something like if(entry.getType().getName() == "article")' which is now replaced byif(entry.getType() == "article")`. So often the name of the BibType is used, which somehow destroys the advantage.

Alternative solution: Store type information as an enum in the entry and convert this enum to a BibType when needed.
This approach keeps all the advantages but does not have the "stringy problem". It feels however a little bit overengineered (mainly because there are "unknown" types which are not covered by a simple enum)

@koppor
Copy link
Member

koppor commented Jan 23, 2016

Refs #551

@simonharrer
Copy link
Contributor

  • TypedBibEntry as a wrapper around BibEntry; TypedBibEntry has high level API, BibEntry has low level API
  • TypedBibEntry -> BibEntry; BibEntry -> RawBibEntry

@tobiasdiez
Copy link
Member Author

Rebased (not sure why there is a merge commit....) and introduced TypedBibEntry aka GoodEasyGodBibEntry (is this the correct way to implement it?).

Todo (other PR): BibDatabase.getResolvedField should accept Optional database.

@simonharrer simonharrer merged commit 7c9b36a into JabRef:master Jan 26, 2016
@tobiasdiez tobiasdiez deleted the entryType branch January 26, 2016 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants