Skip to content

Observable Preferences U#9619

Merged
Siedlerchr merged 2 commits into
mainfrom
abrev_prefs
Mar 6, 2023
Merged

Observable Preferences U#9619
Siedlerchr merged 2 commits into
mainfrom
abrev_prefs

Conversation

@calixtus

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

Copy link
Copy Markdown
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
Copy Markdown
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?

@calixtus calixtus Feb 16, 2023

Copy link
Copy Markdown
Member Author

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

component: preferences dev: code-quality Issues related to code or architecture decisions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants