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 U #9619

Merged
merged 2 commits into from
Mar 6, 2023
Merged

Observable Preferences U #9619

merged 2 commits into from
Mar 6, 2023

Conversation

calixtus
Copy link
Member

Follow up to #9535

  • 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 type: code-quality Issues related to code or architecture decisions preferences labels Feb 14, 2023
@Siedlerchr
Copy link
Member

yeah important step!

Comment on lines -518 to -524
@ParameterizedTest
@MethodSource("provideTestFiles")
void testSaveExternalFilesListToPreferences(TestData testData) throws IOException {
addFourTestFileToViewModelAndPreferences(testData);
verify(preferencesService).storeJournalAbbreviationPreferences(any());
}

Copy link
Member

Choose a reason for hiding this comment

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

From reading the deleted code only, I don't get why it is not replaced by some other testing code -
Shouldn't the preferences then stored be in the abbreviationPreferences?

Copy link
Member Author

@calixtus calixtus Feb 16, 2023

Choose a reason for hiding this comment

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

Thing is, it does not make sense to test here if the private putBoolean method is called. This should be a test in JabRefPreferencesTests or JournalAbbreviationPreferencesTest, but this is somewhat trivial and I assume the behavior of observers is testet within javafx already.

If you really want to test the view model we should certainly not test with any() as an argument.

With other words: the test, as it is, is useless imho.

@calixtus calixtus marked this pull request as ready for review March 6, 2023 19:58
@Siedlerchr Siedlerchr merged commit bcd808b into main Mar 6, 2023
@Siedlerchr Siedlerchr deleted the abrev_prefs branch March 6, 2023 19:58
Siedlerchr added a commit that referenced this pull request Mar 14, 2023
…rg.mariadb.jdbc-mariadb-java-client-3.1.0

* upstream/main: (357 commits)
  Fix syntax
  Add experimental Fetcher for Bibliotheksverbund Bayern with MarcXML parser (#9641)
  Update guidelines-for-setting-up-a-local-workspace.md
  Update guidelines-for-setting-up-a-local-workspace.md
  Bump org.tinylog:slf4j-tinylog from 2.6.0 to 2.6.1 (#9665)
  Bump apple-actions/import-codesign-certs from 1 to 2 (#9662)
  Bump com.puppycrawl.tools:checkstyle from 10.8.0 to 10.8.1 (#9661)
  Bump gittools/actions from 0.9.15 to 0.10.2 (#9663)
  Bump hmarr/auto-approve-action from 3.1.0 to 3.2.0 (#9664)
  Bump io.github.classgraph:classgraph from 4.8.156 to 4.8.157 (#9666)
  Bump org.tinylog:tinylog-api from 2.6.0 to 2.6.1 (#9667)
  Add option to open arks in the browser from an ark identifier (#9601)
  remove "jdk 19 does not work" (#9658)
  Fulltext fetcher for IACR eprints (#9651)
  Observable Preferences S (#9619)
  Issue 9646: Right-click context menu "Attach file from URL" (#9648)
  Improve the INSPIREFetcher in "Update with bibliographic information from the web" (#9645)
  Bump appleboy/ssh-action from 0.1.7 to 0.1.8 (#9653)
  Bump com.fasterxml.jackson.datatype:jackson-datatype-jsr310 (#9656)
  Bump com.puppycrawl.tools:checkstyle from 10.7.0 to 10.8.0 (#9655)
  ...
@calixtus calixtus changed the title Observable Preferences S Observable Preferences U Apr 27, 2023
@koppor koppor mentioned this pull request Jul 12, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preferences 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