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.

Sign up for GitHub

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

Jump to bottom

fix(tests)+refactor(new reviewer): move preferences to Prefs #17829

Merged
merged 1 commit into from
Jan 16, 2025
Merged

fix(tests)+refactor(new reviewer): move preferences to Prefs #17829

merged 1 commit into from
Jan 16, 2025

Conversation

BrayanDSO
Copy link
Member

Approach

Move a pref in each commit

How Has This Been Tested?

I'm trusting the unit tests

Checklist

Please, go through these checks before submitting the PR.

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

@BrayanDSO BrayanDSO added Needs Review cleanup Non functional change that would improve the code readability labels Jan 16, 2025
@BrayanDSO BrayanDSO marked this pull request as draft January 16, 2025 11:50
@BrayanDSO BrayanDSO force-pushed the ref-sets branch 2 times, most recently from 5321d68 to b31205e Compare January 16, 2025 12:45
@BrayanDSO BrayanDSO changed the title refactor(new reviewer): move preferences to Prefs fix(tests)+refactor(new reviewer): move preferences to Prefs Jan 16, 2025
@BrayanDSO BrayanDSO marked this pull request as ready for review January 16, 2025 12:47
@BrayanDSO BrayanDSO added Needs Second Approval Has one approval, one more approval to merge and removed Needs Review labels Jan 16, 2025
david-allison
david-allison requested changes Jan 16, 2025
View reviewed changes
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

The concept of the FIXME looks great, but I think the implementation is flaky

AnkiDroid/src/test/java/com/ichi2/anki/settings/PrefsTest.kt Outdated Show resolved Hide resolved
@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label Jan 16, 2025
@david-allison david-allison added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed Needs Second Approval Has one approval, one more approval to merge labels Jan 16, 2025
@BrayanDSO BrayanDSO marked this pull request as draft January 16, 2025 16:44
@BrayanDSO BrayanDSO added the squash-merge The pull request currently requires maintainers to "Squash Merge" label Jan 16, 2025
@BrayanDSO BrayanDSO removed the Needs Author Reply Waiting for a reply from the original author label Jan 16, 2025
@BrayanDSO
Copy link
Member Author

Tests sometimes feels like illusionism. Idk why I tried to replace JUnit5 with 4, nor why that solved some of the flakiness.

Thanks for the heads up about the lambda. Also thanks to Mike for the flaky hammer (and to GitHub for running it for free because I used it more than I should). I would give up if it didn't exist.

https://github.com/BrayanDSO/Anki-Android/actions/runs/12814915193/job/35732459859

9gwjo8

@BrayanDSO BrayanDSO marked this pull request as ready for review January 16, 2025 18:31
david-allison
david-allison approved these changes Jan 16, 2025
View reviewed changes
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

I know how the yak shaves feel. Android strikes again!

Great going!

@BrayanDSO @david-allison
refactor(reviewer): move preferences to Prefs
e815af1 
+ fix(tests)

fix(tests): Prefs
Sometimes the order wasn't right, and the delegated methods weren't being parsed

refactor: add autoFocusTypeAnswer to Prefs
refactor: add ignoreDisplayCutout to Prefs
refactor: add HideSystemBars to Prefs
@david-allison
Copy link
Member

Squashed as requested. Ready to go

@david-allison david-allison removed the squash-merge The pull request currently requires maintainers to "Squash Merge" label Jan 16, 2025
@david-allison david-allison added this pull request to the merge queue Jan 16, 2025
Merged via the queue into ankidroid:main with commit 50ba4e0 Jan 16, 2025
7 checks passed
@github-actions github-actions bot added this to the 2.21 release milestone Jan 16, 2025
@github-actions github-actions bot removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@david-allison david-allison david-allison approved these changes

@Arthur-Milchior Arthur-Milchior Arthur-Milchior approved these changes

Assignees
No one assigned
Labels
cleanup Non functional change that would improve the code readability
Projects
None yet
Milestone
2.21 release
Development

Successfully merging this pull request may close these issues.
3 participants
@BrayanDSO @david-allison @Arthur-Milchior