Refactor BibEntry deprecated method #4554

Balhau · 2018-12-24T14:45:50Z

Refactored BibEntry deprecated constructor

Siedlerchr

Thanks for your contribution, however it would be nice and more helpful if your PRs would fix an actual issue which is not just purely code quality.
You can look at good first issues to get started. If you need any help or have questions please feel free to ask

LinusDietz · 2018-12-25T19:06:42Z

I'm all in favor to eliminate constructors we have defined as @deprecated.

This is quite a big change, and there are some failing tests. Can you fix these tests, before we review the code?

Balhau · 2018-12-26T11:00:32Z

I'm all in favor to eliminate constructors we have defined as @deprecated.

This is quite a big change, and there are some failing tests. Can you fix these tests, before we review the code?

Sure. I'll fix them as soon as possible!

tobiasdiez · 2018-12-27T11:19:55Z

src/main/java/org/jabref/logic/importer/fileformat/BiblioscapeImporter.java

@@ -255,7 +256,7 @@ public ParserResult importDatabase(BufferedReader reader) throws IOException {
                if (!comments.isEmpty()) { // set comment if present
                    hm.put(FieldName.COMMENT, String.join(";", comments));
                }
-                BibEntry b = new BibEntry(bibtexType);
+                BibEntry b = new BibEntry(BibtexEntryTypes.getType(bibtexType).get());


This may throw a NPE, because BibtexEntryTypes.getType(bibtexType) may return an empty optional. To handle such cases, one would need to add code similar to how you did it in BibtexParser. In order to reuse this code, I would propose to add a getTypeOrDefault method to BibtexEntryTypes.

Thanks. There are still a few other places where the new method should be used, this is also the reason why some of the unit tests fail at the moment.

You're right. Fixed several tests all related with this NPE problem but some are failing. I switch to master and noticed that these are also failing on master so I believe these are non related.

…blem

LinusDietz

Hi, I have one refactoring idea, and please delete the deprecated constructor (Lines 85 --92 of BibEntry)

LinusDietz · 2018-12-28T16:58:54Z

src/main/java/org/jabref/model/entry/BibtexEntryTypes.java

+        if (optType.isPresent()) {
+            return optType.get();
+        }
+        return new CustomEntryType(name,"required","optional");


You can refactor the whole block into

return getType(name).orElseGet(() -> new CustomEntryType(name, "required", "optional"));

I can do that for sure. There is, however a slightly semantic difference. As is you now have the possibility to differentiate between an entry type that is present in the enumeration from one that is defaulted and returned custom. If we merge this logic in the getType method we lose this capability because we will always return something. Or a value from the enumerator or the defaulted one. I'm not the best person to judge if this is a good decision or not because I'm a newbie in the code however if that capability is not needed nor desired I agree that is a good way to cleanup code. What do you think?

Hi! If I got it right, @LinusDietz suggested to replace your code with the one-liner in the getTypeOrDefault() method.

Best,
Matthias

Oh sorry. I misunderstood.

Siedlerchr

Thank you very much for your contribution! If you fix the minor things mentioned by @LinusDietz it's good to go!

matthiasgeiger

One small comment from my side... 😉

matthiasgeiger · 2018-12-28T17:56:08Z

src/test/java/org/jabref/model/BibDatabaseContextTest.java

        db.insertEntry(e1);

        BibDatabaseContext bibDatabaseContext = new BibDatabaseContext(db);
-        assertEquals(BibDatabaseMode.BIBLATEX, bibDatabaseContext.getMode());
+        assertEquals(BibDatabaseMode.BIBTEX, bibDatabaseContext.getMode());


Here you change the logic of the test.
Can you please use 'BiblatexEntryTypes.ELECTRONIC' in line 46 and keep the BibDatabaseMode.BIBLATEX here?

Whoops. good catch!

matthiasgeiger

LGTM now! 👍

tobiasdiez

Thanks again!

* upstream/master: Refactor BibEntry deprecated method (#4554) Added extra stats to be sent with MrDLib recommendations (#4452)

* upstream/master: Bump checkstyle from 8.15 to 8.16 (#4562) Bump xmpbox from 2.0.12 to 2.0.13 (#4561) Delete the deprecated BibEntry Constructor (#4560) Refactor BibEntry deprecated method (#4554) Added extra stats to be sent with MrDLib recommendations (#4452) improve styling of preferences side menu (#4556) Cleanup interfaces (#4553)

* 'ooPanel' of github.com:JabRef/jabref: Refactor BibEntry deprecated method (#4554) Added extra stats to be sent with MrDLib recommendations (#4452) checkstyle change mac default settings to LO path Add Book as preview as well fix style file select layout and inline variable' improve styling of preferences side menu (#4556) Cleanup interfaces (#4553)

* Refactor BibEntry deprecated method * Fixed error * More on checkstyle fixing * Fixed checkstyle issues * Added custom entrytype for types not registered in the enumerator. * Added getTypeOrDefault method refactor code to use it and fix NPE problem * Fixing checkstyle rules * More on checkstyle * More on getType getTypeOrDefault replacement * Revert Article EntryType into Electronic * Added break line between different packages * Refactor BibtextEntryTypes.getTypeOrDefault method * Removed unused import * Removed extra new line, checkstyle error fixing

* upstream/master: (583 commits) update jfoenix and gradle plugins Replace outdated transformer log4j2 with official new one Update journalList.txt Fix for Issue #4437 - Some bugs in preference->Entry table columns (#4546) Don't set column sort type at startup (#4577) Add uncaught exception message (#4565) Converts integrity check dialog to JavaFX (#4559) Do not extract file ending from Urls (#4547) Bump checkstyle from 8.15 to 8.16 (#4562) Bump xmpbox from 2.0.12 to 2.0.13 (#4561) Delete the deprecated BibEntry Constructor (#4560) Refactor BibEntry deprecated method (#4554) Added extra stats to be sent with MrDLib recommendations (#4452) improve styling of preferences side menu (#4556) Cleanup interfaces (#4553) Bump fontbox from 2.0.12 to 2.0.13 (#4552) Bump pdfbox from 2.0.12 to 2.0.13 (#4551) Bump wiremock from 2.19.0 to 2.20.0 (#4550) Fixes that renaming a group did not change the group name in the interface (#4549) Bump applicationinsights-logging-log4j2 from 2.2.1 to 2.3.0 (#4540) Bump antlr4-runtime from 4.7.1 to 4.7.2 (#4542) ... # Conflicts: # src/main/java/org/jabref/gui/JabRefFrame.java # src/main/java/org/jabref/model/entry/BibtexString.java

* Cleanup interfaces (#4553) * improve styling of preferences side menu (#4556) * Added extra stats to be sent with MrDLib recommendations (#4452) * Refactor BibEntry deprecated method (#4554) * Refactor BibEntry deprecated method * Fixed error * More on checkstyle fixing * Fixed checkstyle issues * Added custom entrytype for types not registered in the enumerator. * Added getTypeOrDefault method refactor code to use it and fix NPE problem * Fixing checkstyle rules * More on checkstyle * More on getType getTypeOrDefault replacement * Revert Article EntryType into Electronic * Added break line between different packages * Refactor BibtextEntryTypes.getTypeOrDefault method * Removed unused import * Removed extra new line, checkstyle error fixing * Delete the deprecated BibEntry Constructor (#4560) * Bump xmpbox from 2.0.12 to 2.0.13 (#4561) Bumps xmpbox from 2.0.12 to 2.0.13. Signed-off-by: dependabot[bot] <support@dependabot.com> * Bump checkstyle from 8.15 to 8.16 (#4562) Bumps [checkstyle](https://github.com/checkstyle/checkstyle) from 8.15 to 8.16. - [Release notes](https://github.com/checkstyle/checkstyle/releases) - [Commits](checkstyle/checkstyle@checkstyle-8.15...checkstyle-8.16) Signed-off-by: dependabot[bot] <support@dependabot.com> * Do not extract file ending from Urls (#4547) * Fixes #4544 Do not extract file ending from Urls * Add tests * file type for any resource type * Keep simple file name extraction for files * checkstyle * Converts integrity check dialog to JavaFX (#4559) * Converts integrity check dialog to JavaFX Moreover: - Show entry by reference and not by id. Fixes #2181. - Fixes a few issues that occurred when opening the entry editor by code from the integrity dialog - Reuse gridpane in entry editor (should have a slightly superior performance) - Improve display of progress dialog - Invoke copy files task using central task executor * fix l10n fix aborting of copy files task and showing of integrity check dialog * fix l10n * Add uncaught exception message (#4565) * Add error message for uncaught exceptions Added a new view to the project. It's shown after the uncaught exception is logged. * Add simple text to fallback error view Added a label asking the user to look into the logfiles for more details. * Add error message to language files Added the error message to the german and english language files. * Add ErrorDialogAndWait Removed the FallbackErrorView. Added the showErrorDialogAndWait call instead. * BibTex parser triggered on focus out of the text area instead of on value change event * BibTex parser triggered on focus out of the text area instead of on value change event * Added a post-parsing validation to be sure there's no relevant unconsidered content into epilog. Added DialogService in SourceTab as current NotificationPane seems not working. * Added a post-parsing validation to be sure there's no relevant unconsidered content into epilog. Added DialogService in SourceTab as current NotificationPane seems not working. * Codacy/PR Quality Review change. * Codacy/PR Quality Review change.

9e81857 Update marine-ornithology.csl (JabRef#4566) 938537e Update government-and-opposition.csl (JabRef#4564) 50d0635 Update annales-de-demographie-historique.csl (JabRef#4560) 42b54d9 Create marine-ornithology.csl (JabRef#4559) eff41bb Create dermatology-online.csl (JabRef#4558) 3911696 Update journal-of-forensic-sciences.csl (JabRef#4554) 059ea33 Small capitals and categories (JabRef#4556) 97b8c84 Create CSL Styles: estudios_hispanicos.csl (JabRef#4557) 57e0f9d Update sciences-po-ecole-doctorale-note-french.csl (JabRef#4552) f481ea3 Create ipb-ppki-3.csl (JabRef#4550) 899d302 Update journal-of-neolithic-archaeology.csl (JabRef#4553) e106215 Create annales-de-demographie-historique.csl (JabRef#4512) 4dd974e Create lauterbornia.csl (JabRef#4525) e5277ae Update sciences-po-ecole-doctorale-note-french.csl (JabRef#4551) f9cfe40 Add new IOP dependents (JabRef#4549) daff985 Create brazilian-journal-of-veterinary-research-and-animal-science.csl (JabRef#4544) 10c6fa8 Update ens-de-lyon-centre-d-ingenierie-documentaire.csl (JabRef#4545) git-subtree-dir: src/main/resources/csl-styles git-subtree-split: 9e81857

9e81857 Update marine-ornithology.csl (#4566) 938537e Update government-and-opposition.csl (#4564) 50d0635 Update annales-de-demographie-historique.csl (#4560) 42b54d9 Create marine-ornithology.csl (#4559) eff41bb Create dermatology-online.csl (#4558) 3911696 Update journal-of-forensic-sciences.csl (#4554) 059ea33 Small capitals and categories (#4556) 97b8c84 Create CSL Styles: estudios_hispanicos.csl (#4557) 57e0f9d Update sciences-po-ecole-doctorale-note-french.csl (#4552) f481ea3 Create ipb-ppki-3.csl (#4550) 899d302 Update journal-of-neolithic-archaeology.csl (#4553) e106215 Create annales-de-demographie-historique.csl (#4512) 4dd974e Create lauterbornia.csl (#4525) e5277ae Update sciences-po-ecole-doctorale-note-french.csl (#4551) f9cfe40 Add new IOP dependents (#4549) daff985 Create brazilian-journal-of-veterinary-research-and-animal-science.csl (#4544) 10c6fa8 Update ens-de-lyon-centre-d-ingenierie-documentaire.csl (#4545) git-subtree-dir: src/main/resources/csl-styles git-subtree-split: 9e81857

9e81857 Update marine-ornithology.csl (JabRef#4566) 938537e Update government-and-opposition.csl (JabRef#4564) 50d0635 Update annales-de-demographie-historique.csl (JabRef#4560) 42b54d9 Create marine-ornithology.csl (JabRef#4559) eff41bb Create dermatology-online.csl (JabRef#4558) 3911696 Update journal-of-forensic-sciences.csl (JabRef#4554) 059ea33 Small capitals and categories (JabRef#4556) 97b8c84 Create CSL Styles: estudios_hispanicos.csl (JabRef#4557) 57e0f9d Update sciences-po-ecole-doctorale-note-french.csl (JabRef#4552) f481ea3 Create ipb-ppki-3.csl (JabRef#4550) 899d302 Update journal-of-neolithic-archaeology.csl (JabRef#4553) e106215 Create annales-de-demographie-historique.csl (JabRef#4512) 4dd974e Create lauterbornia.csl (JabRef#4525) e5277ae Update sciences-po-ecole-doctorale-note-french.csl (JabRef#4551) f9cfe40 Add new IOP dependents (JabRef#4549) daff985 Create brazilian-journal-of-veterinary-research-and-animal-science.csl (JabRef#4544) 10c6fa8 Update ens-de-lyon-centre-d-ingenierie-documentaire.csl (JabRef#4545) c531528 create mcgill9-en.csl, Canadian McGill legal style (JabRef#4532) e2416fa Update clinical-and-experimental-optometry.csl (JabRef#4538) git-subtree-dir: src/main/resources/csl-styles git-subtree-split: 9e81857

9e81857 Update marine-ornithology.csl (JabRef#4566) 938537e Update government-and-opposition.csl (JabRef#4564) 50d0635 Update annales-de-demographie-historique.csl (JabRef#4560) 42b54d9 Create marine-ornithology.csl (JabRef#4559) eff41bb Create dermatology-online.csl (JabRef#4558) 3911696 Update journal-of-forensic-sciences.csl (JabRef#4554) git-subtree-dir: src/main/resources/csl-styles git-subtree-split: 9e81857

aa52aaa Encrypt Travis CI notification email addresses (JabRef#4570) d8f2843 Update ayer.csl (JabRef#4569) 448e67e Create ayer.csl (JabRef#4565) bf1ddae Update springer-basic-author-date.csl (JabRef#4562) db525ac Update american-journal-of-physical-anthropology.csl (JabRef#4567) 9e81857 Update marine-ornithology.csl (JabRef#4566) 938537e Update government-and-opposition.csl (JabRef#4564) 50d0635 Update annales-de-demographie-historique.csl (JabRef#4560) 42b54d9 Create marine-ornithology.csl (JabRef#4559) eff41bb Create dermatology-online.csl (JabRef#4558) 3911696 Update journal-of-forensic-sciences.csl (JabRef#4554) 059ea33 Small capitals and categories (JabRef#4556) 97b8c84 Create CSL Styles: estudios_hispanicos.csl (JabRef#4557) 57e0f9d Update sciences-po-ecole-doctorale-note-french.csl (JabRef#4552) f481ea3 Create ipb-ppki-3.csl (JabRef#4550) 899d302 Update journal-of-neolithic-archaeology.csl (JabRef#4553) e106215 Create annales-de-demographie-historique.csl (JabRef#4512) 4dd974e Create lauterbornia.csl (JabRef#4525) e5277ae Update sciences-po-ecole-doctorale-note-french.csl (JabRef#4551) f9cfe40 Add new IOP dependents (JabRef#4549) daff985 Create brazilian-journal-of-veterinary-research-and-animal-science.csl (JabRef#4544) 10c6fa8 Update ens-de-lyon-centre-d-ingenierie-documentaire.csl (JabRef#4545) git-subtree-dir: src/main/resources/csl-styles git-subtree-split: aa52aaa

f03dece Bump nokogiri from 1.10.7 to 1.10.8 (JabRef#4580) 31bacc1 JCL: Fix formatting & space before locator adfc9b0 Add The Journal of Comparative Law (JabRef#4576) 599d39c Create uni-freiburg-geschichte.csl (JabRef#4561) 3ad8d40 Add Begell House styles (JabRef#4575) 223527b Create begell-apa.csl (JabRef#4574) d73b9a9 Update journal-of-neolithic-archaeology.csl (JabRef#4572) 5b2810c Create anatomical-sciences-education.csl (JabRef#4573) 9454897 Update ASA style b3c6efd Reorder/reindent (JabRef#4571) aa52aaa Encrypt Travis CI notification email addresses (JabRef#4570) d8f2843 Update ayer.csl (JabRef#4569) 448e67e Create ayer.csl (JabRef#4565) bf1ddae Update springer-basic-author-date.csl (JabRef#4562) db525ac Update american-journal-of-physical-anthropology.csl (JabRef#4567) 9e81857 Update marine-ornithology.csl (JabRef#4566) 938537e Update government-and-opposition.csl (JabRef#4564) 50d0635 Update annales-de-demographie-historique.csl (JabRef#4560) 42b54d9 Create marine-ornithology.csl (JabRef#4559) eff41bb Create dermatology-online.csl (JabRef#4558) 3911696 Update journal-of-forensic-sciences.csl (JabRef#4554) 059ea33 Small capitals and categories (JabRef#4556) 97b8c84 Create CSL Styles: estudios_hispanicos.csl (JabRef#4557) 57e0f9d Update sciences-po-ecole-doctorale-note-french.csl (JabRef#4552) f481ea3 Create ipb-ppki-3.csl (JabRef#4550) 899d302 Update journal-of-neolithic-archaeology.csl (JabRef#4553) e106215 Create annales-de-demographie-historique.csl (JabRef#4512) 4dd974e Create lauterbornia.csl (JabRef#4525) e5277ae Update sciences-po-ecole-doctorale-note-french.csl (JabRef#4551) f9cfe40 Add new IOP dependents (JabRef#4549) daff985 Create brazilian-journal-of-veterinary-research-and-animal-science.csl (JabRef#4544) 10c6fa8 Update ens-de-lyon-centre-d-ingenierie-documentaire.csl (JabRef#4545) git-subtree-dir: src/main/resources/csl-styles git-subtree-split: f03dece

Balhau added 3 commits December 24, 2018 14:43

Refactor BibEntry deprecated method

1632efd

Fixed error

b281436

More on checkstyle fixing

c13d9f5

Siedlerchr reviewed Dec 24, 2018

View reviewed changes

Fixed checkstyle issues

38248dd

Added custom entrytype for types not registered in the enumerator.

2458fbb

tobiasdiez reviewed Dec 27, 2018

View reviewed changes

Balhau added 4 commits December 28, 2018 11:14

Added getTypeOrDefault method refactor code to use it and fix NPE pro…

f6cfd28

…blem

Fixing checkstyle rules

1d02074

More on checkstyle

80b2650

More on getType getTypeOrDefault replacement

4e00355

LinusDietz requested changes Dec 28, 2018

View reviewed changes

LinusDietz added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers type: code-quality Issues related to code or architecture decisions labels Dec 28, 2018

Siedlerchr approved these changes Dec 28, 2018

View reviewed changes

matthiasgeiger suggested changes Dec 28, 2018

View reviewed changes

Balhau added 5 commits December 28, 2018 18:16

Revert Article EntryType into Electronic

85ddbf5

Added break line between different packages

a5c092c

Refactor BibtextEntryTypes.getTypeOrDefault method

46b4323

Removed unused import

db0ddcb

Removed extra new line, checkstyle error fixing

6233c6b

matthiasgeiger approved these changes Dec 28, 2018

View reviewed changes

tobiasdiez approved these changes Dec 28, 2018

View reviewed changes

tobiasdiez merged commit cd1e795 into JabRef:master Dec 28, 2018

LinusDietz mentioned this pull request Dec 29, 2018

Delete the Deprecated BibEntry Constructor #4560

Merged

Siedlerchr added a commit that referenced this pull request Dec 29, 2018

Merge remote-tracking branch 'upstream/master' into ooPanel

e83078e

* upstream/master: Refactor BibEntry deprecated method (#4554) Added extra stats to be sent with MrDLib recommendations (#4452)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor BibEntry deprecated method #4554

Refactor BibEntry deprecated method #4554

Balhau commented Dec 24, 2018

Siedlerchr left a comment

LinusDietz commented Dec 25, 2018

Balhau commented Dec 26, 2018

tobiasdiez Dec 27, 2018

Balhau Dec 28, 2018

tobiasdiez Dec 28, 2018

Balhau Dec 28, 2018

LinusDietz left a comment

LinusDietz Dec 28, 2018

Balhau Dec 28, 2018 •

edited

Loading

matthiasgeiger Dec 28, 2018

Balhau Dec 28, 2018

Siedlerchr left a comment

matthiasgeiger left a comment

matthiasgeiger Dec 28, 2018

LinusDietz Dec 28, 2018

Balhau Dec 28, 2018

matthiasgeiger left a comment

tobiasdiez left a comment

Refactor BibEntry deprecated method #4554

Refactor BibEntry deprecated method #4554

Conversation

Balhau commented Dec 24, 2018

Siedlerchr left a comment

Choose a reason for hiding this comment

LinusDietz commented Dec 25, 2018

Balhau commented Dec 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LinusDietz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Balhau Dec 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr left a comment

Choose a reason for hiding this comment

matthiasgeiger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiasgeiger left a comment

Choose a reason for hiding this comment

tobiasdiez left a comment

Choose a reason for hiding this comment

Balhau Dec 28, 2018 •

edited

Loading