search

dkpro / dkpro-jwktl

Java Wiktionary Library
http://dkpro.org/dkpro-jwktl/
Apache License 2.0
57 stars 26 forks source link

Isolation of berkeley for better usage of other DBs #49

Closed rafaelhoff closed 3 years ago

rafaelhoff commented 6 years ago

Hello,

I have been working with some friends with dkpro-jwktl for some time in a PoC. We made some changes for testing new DBs. This is a pull-request that removes parts of the coupling of the whole project with Berkeley itself. It makes a little bit easier to replace to a different DB.

codecov-io commented 6 years ago

Codecov Report

Merging #49 into master will increase coverage by 0.26%. The diff coverage is 75.18%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #49      +/-   ##
============================================
+ Coverage     75.99%   76.26%   +0.26%     
- Complexity     1321     1341      +20     
============================================
  Files           101      103       +2     
  Lines          4629     4757     +128     
  Branches        801      818      +17     
============================================
+ Hits           3518     3628     +110     
- Misses          895      907      +12     
- Partials        216      222       +6
Impacted Files Coverage Δ Complexity Δ
...mstadt/ukp/jwktl/api/entry/WiktionaryWordForm.java 93.54% <ø> (ø) 19 <0> (ø) :arrow_down:
...wktl/db/berkeley/BerkeleyDBWiktionaryIterator.java 60.86% <ø> (ø) 4 <0> (?)
...de/tudarmstadt/ukp/jwktl/api/entry/WikiString.java 56% <ø> (ø) 8 <0> (ø) :arrow_down:
...darmstadt/ukp/jwktl/api/entry/WiktionaryEntry.java 78.41% <ø> (ø) 61 <0> (ø) :arrow_down:
...adt/ukp/jwktl/api/entry/WiktionaryTranslation.java 88.88% <ø> (ø) 16 <0> (ø) :arrow_down:
...mstadt/ukp/jwktl/api/entry/WiktionaryRelation.java 57.14% <ø> (ø) 5 <0> (ø) :arrow_down:
...rmstadt/ukp/jwktl/api/entry/WiktionaryExample.java 90% <ø> (ø) 5 <0> (ø) :arrow_down:
...tudarmstadt/ukp/jwktl/api/entry/Pronunciation.java 100% <ø> (ø) 5 <0> (ø) :arrow_down:
src/main/java/de/tudarmstadt/ukp/jwktl/JWKTL.java 30.76% <ø> (ø) 4 <0> (ø) :arrow_down:
...udarmstadt/ukp/jwktl/api/entry/WiktionaryPage.java 83.05% <ø> (ø) 26 <0> (ø) :arrow_down:
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update db23009...bf2b7fa. Read the comment docs.

chmeyer commented 6 years ago

Nice changes. I have added a small comment regarding exception handling. In order to move forward with factoring out the Berkeley DB in the future, we could rename the jwktl.api.berkeley package to jwktl.db.berkeley. This way, it would be possible to easily divide JWKTL into a multi-module Maven project with 3 modules api, db, and parser within a future release. What do you think?

In order to merge the PR then, we will need a signed contributor's license, see: https://dkpro.github.io/contributing/ Could you please sign it and send it by mail the address in the license (and to me in CC)?

rafaelhoff commented 6 years ago

The change of the namespace makes sense. To separate into different modules was also my idea, but if I would send a PR, it would mean almost removal of code only. :-) I sent the CLA per e-mail.

chmeyer commented 6 years ago

CLA well received, thanks. We should definitely do the module separation as a bigger step, when starting a new version. But what we could do right now is changing the package name from jwktl.api.berkeley package to jwktl.db.berkeley. This way, it would be clearer that api, parser, and db can go to separate modules in the future. Could you change that and throwing the exception BerkeleyConfigurationModel:75 this PR before merging?

rafaelhoff commented 5 years ago

hello @chmeyer, anything missing for the pull request now?