Skip to content

Rewrite code of MedlineImporter#9673

Merged
koppor merged 8 commits into
JabRef:mainfrom
aqurilla:fix-for-issue-9536
Mar 18, 2023
Merged

Rewrite code of MedlineImporter#9673
koppor merged 8 commits into
JabRef:mainfrom
aqurilla:fix-for-issue-9536

Conversation

@aqurilla

@aqurilla aqurilla commented Mar 16, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

This fixes #9536 by rewriting the org.jabref.logic.importer.fileformat.MedlineImporter class to use a StAX-Parser, and removes all JAXB dependencies from the class. The corresponding xjc task is also removed from build.gradle.

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@aqurilla aqurilla marked this pull request as ready for review March 16, 2023 06:53
@Siedlerchr

Siedlerchr commented Mar 16, 2023

Copy link
Copy Markdown
Member

Thanks! Looks good! can you take a look at #4273 as well?

@aqurilla

aqurilla commented Mar 16, 2023

Copy link
Copy Markdown
Contributor Author

Sure, I'll check that one!

koppor
koppor requested changes Mar 16, 2023
View reviewed changes

@koppor koppor left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you so much for working on this! 🎉

I have micro nitpicks 😇 - otherwise, looks good!

Comment thread src/main/java/org/jabref/logic/importer/fileformat/MedlineImporter.java Outdated
Comment thread build.gradle
Comment thread src/test/resources/org/jabref/logic/importer/fileformat/MedlineImporterTestNbib.bib
Comment thread src/main/java/org/jabref/logic/importer/fileformat/medline/ArticleID.java
@aqurilla

aqurilla commented Mar 17, 2023

Copy link
Copy Markdown
Contributor Author

I think the AstrophysicsDataSystemTest failed due to some network issues? I did not make any changes related to those files

@ThiloteE ThiloteE added dev: code-quality Issues related to code or architecture decisions component: import-load labels Mar 17, 2023
@koppor

koppor commented Mar 17, 2023

Copy link
Copy Markdown
Member

Yes, their servers return server error:

java.io.IOException: org.jabref.logic.importer.FetcherServerException: Encountered HTTP Status Code 504

@koppor

koppor commented Mar 17, 2023

Copy link
Copy Markdown
Member

I would go ahead to merge this as is - and do fixing of #4273 as follow-up?

@aqurilla

aqurilla commented Mar 18, 2023

Copy link
Copy Markdown
Contributor Author

I found that Abstract and Title xml elements containing italics, bold tags etc. were getting cut short, so updated the code to handle this.
Agreed that MathML support should be a separate PR since it looks like it will require handling a large number of tags!

koppor
koppor approved these changes Mar 18, 2023
View reviewed changes

@koppor koppor left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Think, it's an improvement and I go forward with merge.

Regarding the italics:

The apolipoprotein E (
                        <i>APOE</i>) gene on chromosome

should get

The apolipoprotein E (\textit{APOE}) gene on chromosome

Since it are very rare cases of HTML tags, it should be "easy". Maybe, this can be implemented when converting MathML?

@koppor koppor merged commit 23a0c44 into JabRef:main Mar 18, 2023
This was referenced Mar 18, 2023
Closed
Closed
Merged
Closed
Siedlerchr added a commit that referenced this pull request Mar 18, 2023
@Siedlerchr
Merge remote-tracking branch 'upstream/main' into restapi
ba80857 
* upstream/main: (26 commits)
  Remove bibtexml.xsd (We don't support BibTeXML any more)
  Rewrite code of MedlineImporter (#9673)
  Rename variable and add more documentation
  Fix checkstyle
  Add hanling of "field = {content\}"
  Add link to PR
  Add link to PR.
  JabRef writes a new backup file only if there is a change.
  Add debug output to log.txt
  Refine logging howto
  Change log level for non-found files during PDF indexing
  Initial ImportHandlerTest
  Streamline AppDirs paths
  add a clear history button in sub-menu
  Removed redundant list
  add for en.properties
  add Localization.lang for string
  correct changelog
  add test for getLastSearchHistory and change HashLinkedSet to ArrayList
  fix the checkstlye
  ...
@koppor koppor mentioned this pull request May 8, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@koppor koppor koppor approved these changes

Assignees

No one assigned

Labels

component: import-load dev: code-quality Issues related to code or architecture decisions

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Rewrite code of MedlineImporter

4 participants

@aqurilla @Siedlerchr @koppor @ThiloteE