Preserve user comments in bib file #1471
Conversation
lenhard
force-pushed
the
preserve-comments
branch
from
e00aeae to
14db4bf
Compare
|
Ok, I'd like to put this up for discussion. User comments are now kept under the following circumstances:
This is independent of whether an entry is changed/reformatted, but not if it is deleted. If it is reformated, user comment text is located exactly one blank line above the reformated entry. User comments are still not kept when:
Is this good enough? It should be sufficient in most scenarios and work if someone opens his non-JabRef bib file with JabRef. If someone starts adding arbitrary text between JabRef's Metadata, though... The latter is hard to achieve since we do not store the serialization of MetaData. If we want to, I'd have to make some additions to our MetaData objects. I could also store the comments between meta data and add them as a bunch at the end of the file, but then the relative position gets lost. Before I do more work, I'd like to clarify the following points:
|
lenhard
added
the
status: ready-for-review
# Conflicts: # CHANGELOG.md
| @@ -12,6 +12,7 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `# | |||
| ## [Unreleased] | |||
|
|
|||
| ### Changed | |||
| - JabRef does no longer delete user comments outside of BibTeX entries [#1026] | |||
There was a problem hiding this comment.
Change format of changelog entry to match the other ones.
|
LGTM. I think integration tests are more valuable. So this would suffice for me. |
|
Great! Let us talk briefly about this during the next devcall and decide if we take it into 3.5 or 3.6. |
|
For 3.6 |
| @@ -369,7 +369,7 @@ return true;</string> | |||
| </serializedBean> | |||
| <condition>context.getBooleanVariable("addToDockAction")</condition> | |||
| </action> | |||
| <group name="Registry" id="239" customizedId="" beanClass="com.install4j.runtime.beans.groups.ActionGroup" enabled="true" commentSet="false" comment="" actionElevationType="inherit"> | |||
| <group name="Registry" id="239" customizedId="" beanClass="com.install4j.runtime.beans.groups.ActionGroup" enabled="true" commentSet="false" comment="" actionElevationType="elevated"> | |||
There was a problem hiding this comment.
Why is an install4j change part of this PR?
There was a problem hiding this comment.
I revert it later, sorry...
| /* | ||
| * Returns user comments (arbitrary text before the string) if there are any. If not returns the empty string | ||
| */ | ||
| public String getUserComments() { |
There was a problem hiding this comment.
This is now an exact copy of BibEntry.getUserComments right? Maybe add a superclass BibItem containing this method and then BibEntry and BibString derive from this superclass (maybe there is even more common code which could be extracted to the super class)
There was a problem hiding this comment.
Sorry to always slip into the role of your antagonist, but I am against creating a class hierarchy. In most cases, it makes the code harder to understand, and the code duplication savings are just not worth the effort. In addition, one has to adhere to the liskov substitution principle to create a good class hierarchy, but this is hard to do right.
There was a problem hiding this comment.
No problem, having an experienced antagonist is probably the best way to learn something 😄
So now ignoring everything about the actual implementation and only speaking about "business objects": There are different items which can be contained in a bib file. For example @Comments, @Strings and normal bib entries. They all have a similar structure (e.g begin with @, followed by some identifier and then braces which surround the actual content) and all of them could have some text comments in front of them. Thus there is also a common behavior when it comes to parsing and writing.
So how would my deer antagonist reflect this common structure and behavior in java code?
My idea would be: BibString, BibEntry, BibComment all derive from BibItem. BibItem has methods to parse and write at least the form @something { abstract method to parse / write value }. Then the writer just gets a list of BibItems and invokes the write method on them. Similarly the parser returns a list of BibItems while the parsing of every item was done in the subclass. But again...this idea somehow corresponds from a service implementation to a full-blown domain model....so yeah, maybe we should discuss this in the next dev call ;-)
|
@lenhard Please squash the commits via Github when merging this 😄 |
|
|
||
| BibEntry entry = entries.iterator().next(); | ||
| assertEquals("test", entry.getCiteKey()); |
There was a problem hiding this comment.
Why did you removed this (suboptimal) assertions?
There was a problem hiding this comment.
Are you being serious? In this very same PR you asked me to reduce the number asserts as far as possible, aiming for one assertion per test. This is the reason.
So I am finally having my take at #1026. The current solution was surprisingly easy so far (despite what I had written before). It looks explicitly for our encoding prefix in the file ("Encoding: ") and kills lines that contain it (but only lines preceding an entry, it will not delete something that is inside an entry). Other user comments are left untouched and are always written out again, even if the entry is reformatted.
User comments that are above meta data, bibtex strings, or the preamble will still be removed, though. Changing that would require large scale changes to these items in our model, since we would also need to store the parsed serialization for them, which we do not do so far.
This should receive significant automated, but also manual, testing, since it modifies a quite critical part and has the potential to destroy bib files. I will add a few more tests for this PR.