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

Mark some methods as deprecated in BibEntry and BibDatabase #1913

Merged
merged 3 commits into from
Sep 4, 2016

Conversation

tobiasdiez
Copy link
Member

Changes are only in BibEntry and BibDatabase (the rest are propagated renames / method signature changes).

  • Change in CHANGELOG.md described
  • Tests created for changes
  • Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)

@tobiasdiez tobiasdiez 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 Sep 3, 2016
// If this field is not set, and the entry has a crossref, try to look up the
// field in the referred entry: Do not do this for the bibtex key.
if (!result.isPresent() && (database != null)) {
Optional<String> crossrefKey = getField(FieldName.CROSSREF);
Copy link
Member

@Siedlerchr Siedlerchr Sep 3, 2016

Choose a reason for hiding this comment

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

Better use a flatmap for chaining optionals, it would only get evaluated if a value is present in all fields and addtionally you can add a filter condition for checking !isEmpty
http://www.nurkiewicz.com/2013/08/optional-in-java-8-cheat-sheet.html (scroll down to end to see the "bigger example")

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, changed the code accordingly.

@Siedlerchr
Copy link
Member

What is the reason behind separating the insert and the duplication check?

@tobiasdiez
Copy link
Member Author

Most of the callers of insertEntry don't use the returned value of the duplication check. Moreover, I find it more readable if the caller first checks if a duplicate already exists (and handles this case appropriately) and then inserts the entry in the db.

@Siedlerchr
Copy link
Member

Thanks for the explaination. Now code LGTM 👍

@lenhard
Copy link
Member

lenhard commented Sep 4, 2016

Ok, this sounds reasonable and the code that I looked at (in BibEntry and BibDatabase, as you suggested) looks good. I am merging it in.

@lenhard lenhard merged commit a9eb978 into JabRef:master Sep 4, 2016
@tobiasdiez tobiasdiez deleted the smallRevert branch September 4, 2016 16:04
@oscargus
Copy link
Contributor

oscargus commented Sep 5, 2016

As commented in the source, the deprecation of constants misses the target. By far.

@lenhard
Copy link
Member

lenhard commented Sep 5, 2016

My bad for merging! The changes in methods still have some values, don't they? So, instead of reverting, how about just removing the deprecation marking for the constants again?

@tobiasdiez What do you think?

@oscargus
Copy link
Contributor

oscargus commented Sep 5, 2016 via email

@lenhard lenhard added the status: waiting-for-feedback The submitter or other users need to provide more information about the issue label Sep 5, 2016
tobiasdiez pushed a commit that referenced this pull request Sep 5, 2016
* Check integrity edition check

* Conflict in Jabref_fr

* Changed order Biblatex/Bibtex condition

* Create own object

* Rename variable

* Extract checker to own file

* Improved comment

* French localization: Jabref_fr: empty strings translated + removal of unused header (#1911)

* Remove teamscale findings in the database package (#1577)

* Check integrity edition check

* Changed order Biblatex/Bibtex condition

* Create own object

* Rename variable

* Mark some methods as deprecated in BibEntry and BibDatabase (#1913)

* Mark some methods as deprecated in BibEntry and BibDatabase

* Rename getResolvedFieldOrAlias

* Use flatmap

* Fix location field not exported correctly to office 2007 xml (#1909)

* Fix location field not exported to office 2007 xml
* Add some test for exporting location and address field
Address in xml field is now imported as location
add some javadoc

* Add possibility to remember password for shared databases. (#1846)

* Add possibility to remember password.
- Add checkbox to the open shared database dialog
- Add SharedDatabasePreferences
- Add password encrypting and decrypting methods
- Update localization entries
- Reorganize clearing methods for Preferences

* Change prefs node and add class comment.

* Relativate node path for password storage.

* Fix LOGGER factory.

* Improve password encryption using XOR.

- Use username as one operand
- Add new parameter to the constructor

* Extract method.

* Improve exception handling.

* Improve password encryption and decryption.

- Use CBC and padding
- Hash the key before using
- Simplify conversion

* Fix modifier. Fix conflicts.

* Extract checker to own file

* Improved comment

* Translation of shared (#1919)

* Add missing entries in german localization

* Resolved conflicts

* Some more conflicts de sv

* Expressions changed
@oscargus
Copy link
Contributor

Another comment: the fact that the duplication checker is not called when inserting the entry means that the keys do not add additional letters after it.

Yes, the callers did more than often not check the return value but now one will have to perform two operations for properly inserting an entry into a database. Both insert it into the database and into the duplicationchecker.

@oscargus
Copy link
Contributor

Not to mention the fact that the duplication checker will not find duplicates inserted the new way (as far as I can tell).

@oscargus
Copy link
Contributor

Good practice when marking things as deprecated: make some simple changes of the deprecated function to the non-deprecated function. Here, it would be obvious that the key-tests failed, despite no actual need to check for any duplicates before inserting them. Also, it would be obvious that most of the constants were not used in a bad way etc.

@lenhard lenhard added status: devcall and removed status: waiting-for-feedback The submitter or other users need to provide more information about the issue labels Sep 14, 2016
@lenhard
Copy link
Member

lenhard commented Sep 14, 2016

Good thing that you bring this back up! It seems it slipped under during this weeks devcall, but we should talk about it again during the next one (propably next week).

EDIT: This PR easily slips under as github hides close/merged stuff. I'll take the link explicitly into the minutes for the next meeting

@Siedlerchr
Copy link
Member

I recenlty discoverd two problems:
The problem with this is deprecated is, that some of the constants are heavily used in a static manner in importers/exporters/fetchers to just check if the current processed field is the bibtexkeyfield.

Second problem: I recently tried to replace getText with getResolvedField in a method and suddenly all kind of test failed....(Unfortunately I don't remember the PR).
So these are not equal/have the same behaviour

@@ -157,8 +151,14 @@ public static boolean isInternalField(String field) {
* @return false if the insert was done without a duplicate warning
* @throws KeyCollisionException thrown if the entry id ({@link BibEntry#getId()}) is already present in the database
*/
public synchronized boolean insertEntry(BibEntry entry) throws KeyCollisionException {
return insertEntry(entry, EntryEventSource.LOCAL);
@Deprecated // use insertEntry and DuplicationChecker separately
Copy link
Member

Choose a reason for hiding this comment

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

Please move the comment to @deprecated javadoc

@@ -222,10 +220,11 @@ public int getNumberOfKeyOccurrences(String key) {
*
* @return true, if the entry contains the key, false if not
*/
@Deprecated // use BibEntry.setCiteKey (and DuplicationChecker)
Copy link
Member

Choose a reason for hiding this comment

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

@deprecated JavaDoc?

* Returns a text with references resolved according to an optionally given database.
*
* @param toResolve maybenull The text to resolve.
* @param database maybenull The database to use for resolving the text.
* @return The resolved text or the original text if either the text or the database are null
*/
@Deprecated // use BibDatabase.resolveForStrings
Copy link
Member

Choose a reason for hiding this comment

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

@deprecated JavaDoc?

koppor added a commit that referenced this pull request Sep 21, 2016
…1913)"

This reverts commit a9eb978.

# Conflicts:
#	src/main/java/net/sf/jabref/logic/importer/fileformat/PdfContentImporter.java
#	src/main/java/net/sf/jabref/model/database/BibDatabase.java
#	src/main/java/net/sf/jabref/model/entry/BibEntry.java
#	src/test/java/net/sf/jabref/logic/search/DatabaseSearcherTest.java
@koppor koppor mentioned this pull request Sep 21, 2016
@koppor
Copy link
Member

koppor commented Sep 21, 2016

I reverted it at #2023. Please comment, directly improve, or merge. :octocat:

koppor added a commit that referenced this pull request Sep 22, 2016
…1913)" (#2023)

This reverts commit a9eb978.

# Conflicts:
#	src/main/java/net/sf/jabref/logic/importer/fileformat/PdfContentImporter.java
#	src/main/java/net/sf/jabref/model/database/BibDatabase.java
#	src/main/java/net/sf/jabref/model/entry/BibEntry.java
#	src/test/java/net/sf/jabref/logic/search/DatabaseSearcherTest.java
zesaro pushed a commit to zesaro/jabref that referenced this pull request Nov 22, 2016
…abRef#1913)" (JabRef#2023)

This reverts commit a9eb978.

# Conflicts:
#	src/main/java/net/sf/jabref/logic/importer/fileformat/PdfContentImporter.java
#	src/main/java/net/sf/jabref/model/database/BibDatabase.java
#	src/main/java/net/sf/jabref/model/entry/BibEntry.java
#	src/test/java/net/sf/jabref/logic/search/DatabaseSearcherTest.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants