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

Observable Preferences L (GrobidPreferences) #9065

Merged
merged 9 commits into from
Aug 16, 2022
Merged

Conversation

calixtus
Copy link
Member

Follow up on #8422 to keep the big refactor going.
Stopping here to keep it reviewable

  • Cleaned up a bit CustomImporter stuff and duplicated code
  • Extracted GrobidPreferences
  • Introduced JabRefPreferences::getSeries
  • Cleaned a bit up in JabRefPreferences

.

  • 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.

@calixtus calixtus 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 preferences labels Aug 16, 2022
Comment on lines +776 to +778
} catch (IOException ignored) {
// Ignored
}
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't ignore this, because if it fails then preferences may have been corrupted.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just copied it from getStringList. Also Exception occurs only, if the stringreader is fails to read a line in the prefs. Worst case is that a preference option is not loaded correctly and has to be reconfigured manually.

int i = 0;
List<String> series = new ArrayList<>();
String item;
while (!StringUtil.isBlank(item = get(key + i))) {
Copy link
Member

Choose a reason for hiding this comment

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

StringUtil#isNotBlank?

Copy link
Member Author

Choose a reason for hiding this comment

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

grafik

Copy link
Member Author

@calixtus calixtus Aug 16, 2022

Choose a reason for hiding this comment

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

isNotBlank is basically a method to be used as a reference in .map(StringUtil::isNotBlank) like Objects::isNull

Copy link
Member

Choose a reason for hiding this comment

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

But still adding a literal 'not' reads better than '!'

Copy link
Member

@HoussemNasri HoussemNasri left a comment

Choose a reason for hiding this comment

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

LGTM!

@Siedlerchr Siedlerchr merged commit a3a886e into main Aug 16, 2022
@Siedlerchr Siedlerchr deleted the cleanup-custom-importer branch August 16, 2022 19:38
Siedlerchr added a commit that referenced this pull request Aug 21, 2022
* upstream/main:
  AtomicFileOutputStream does not overwrite file if exception occurred during write (#9067)
  Observable Preferences L (GrobidPreferences) (#9065)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preferences 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.

3 participants