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

Rewrite bibtexml importer with JAXB parser #1666

Merged
merged 7 commits into from Aug 18, 2016

Conversation

tschechlovdev
Copy link
Contributor

@tschechlovdev tschechlovdev commented Aug 2, 2016

Regarding: #898
I've rewritten the bibtexml importer: It was written with a SAX parser and I've used a JAXB parser. I will be addings some more tests for the changes I've made.

Note: Exporting in bibtexml format and then trying to import in bibtexml format is not working because of #1665.

  • Tests created for changes
  • Manually tested changed features in running JabRef

bibEntry.setType("unpublished");
parseUnpublished(entry.getUnpublished(), fields);
}
if (entry.getId() != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This if condition shouldn't be necessary as the xsd schema file specifies the attribute as use="required"

@boceckts boceckts changed the title Rewrite bibtexml importer with JAXB parser [WIP] Rewrite bibtexml importer with JAXB parser Aug 2, 2016
if (method.getName().equals("getNumber")) {
putNumber(fields, (BigInteger) method.invoke(t));
continue;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Use else if

* "abstract" will be put as key to fields and the value of <Code>getAbstract</Code> will be put as value to fields.
* Some <Code>get</Code> methods shouldn't be mapped to fields, so <Code>getClass</Code> for example will be skipped.
*
* @param t
Copy link
Member

Choose a reason for hiding this comment

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

Either write a parameter description or leave the tag out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also rename, maybe entryType is fitting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed it to entryType and added a description.

@boceckts boceckts added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed stupro-ready-for-internal-review labels Aug 11, 2016
@tschechlovdev tschechlovdev changed the title [WIP] Rewrite bibtexml importer with JAXB parser Rewrite bibtexml importer with JAXB parser Aug 11, 2016
}
return new ParserResult(bibItems);
}

/**
* In this method, all <Code>get</Code> methods that t has will be used and their value will be put to fields,
Copy link
Member

Choose a reason for hiding this comment

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

t -> entryType

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@koppor koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 12, 2016
@tschechlovdev tschechlovdev added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 15, 2016
@tschechlovdev
Copy link
Contributor Author

Are there any other remarks?

} catch (javax.xml.parsers.ParserConfigurationException e) {
JAXBContext context = JAXBContext.newInstance("net.sf.jabref.importer.fileformat.bibtexml");
XMLInputFactory xmlInputFactory = XMLInputFactory.newFactory();
XMLStreamReader xmlReader = xmlInputFactory.createXMLStreamReader(reader);
Copy link
Member

Choose a reason for hiding this comment

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

Why can't directly the reader used as input for the unmarshaller?

The JavaDocs of the unmarshaller say so: https://jaxb.java.net/nonav/2.2.4/docs/api/javax/xml/bind/Unmarshaller.html

Reading that, the code would be much shorter and especially lines 106 to 109 would be obsolete

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I've used directly the reader as input.

# Conflicts:
#	src/main/java/net/sf/jabref/logic/importer/fileformat/BibTeXMLImporter.java
#	src/test/java/net/sf/jabref/logic/importer/fileformat/BibTeXMLImporterTest.java
@koppor koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 17, 2016
@tschechlovdev
Copy link
Contributor Author

Travis is failing because of UI tests...

@tschechlovdev tschechlovdev added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 17, 2016
@koppor
Copy link
Member

koppor commented Aug 17, 2016

Please not that the UI tests are done but ignored. Please read the travis output and check for red lines

Checkstyle failed. Your IDE settings seem to be non-formant. Please double check: https://github.com/JabRef/jabref/wiki/Guidelines-for-setting-up-a-local-workspace#intellij

:checkstyleMain[ant:checkstyle] [ERROR] /home/travis/build/JabRef/jabref/src/main/java/net/sf/jabref/logic/importer/fileformat/BibTeXMLImporter.java:36: 'net.sf.jabref.importer.fileformat.bibtexml.Entry' should be separated from previous imports. [ImportOrder]

 FAILED

FAILURE: Build failed with an exception.

* What went wrong:

Execution failed for task ':checkstyleMain'.

> Checkstyle rule violations were found. See the report at: file:///home/travis/build/JabRef/jabref/build/reports/checkstyle/main.html

* Try:

Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output.

BUILD FAILED

Total time: 1 mins 16.912 secs

@koppor koppor merged commit 7597821 into JabRef:master Aug 18, 2016
@tschechlovdev tschechlovdev mentioned this pull request Aug 18, 2016
Siedlerchr added a commit to Siedlerchr/jabref that referenced this pull request Aug 18, 2016
* master:
  Fix imports
  Rename NewFileDialog -> FileDialog
  Also cancel duplicate finder workflow on close button
  Removed/moved preferences which are constants
  implements JabRef#1767: Add Help Button to access new help page
  Fixed BibTeXMLImporter
  Set more default file filters in dialogs JabRef#1763
  Resolve crossrefs and strings in main table (JabRef#1644)
  Rewrite bibtexml importer with JAXB parser (JabRef#1666)
  Moved a few more initialization to GUIGlobals.init() (JabRef#1756)
  Added program to generate a table of all characters and fixed some characters (JabRef#1766)
  Improve focus of the maintable after a sidepane gets closed (JabRef#1741)
  Table row height adjusts on Windows as the font scales with the menu (JabRef#1623)
  Added more characters to converters (JabRef#1761)
ayanai1 pushed a commit to ayanai1/jabref that referenced this pull request Sep 5, 2016
* rewrite bibtexml importer with jaxb parser

* address comments

* remove unused import

* include feedback

* fix import order, log and add testfile
@tschechlovdev tschechlovdev deleted the bibtexml-importer branch September 20, 2016 09:50
@koppor koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Nov 21, 2016
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.

None yet

3 participants