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

NF: use "note type" in androidTest #17711

Merged
merged 1 commit into from
Jan 4, 2025
Merged

NF: use "note type" in androidTest #17711

merged 1 commit into from
Jan 4, 2025

Conversation

Arthur-Milchior
Copy link
Member

No description provided.

david-allison
david-allison approved these changes Jan 3, 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.

Nothing breaking

AnkiDroid/src/test/java/com/ichi2/anki/AbstractFlashcardViewerTest.kt Outdated
@@ -150,7 +150,7 @@ class AbstractFlashcardViewerTest : RobolectricTest() {
fun testEditingCardChangesTypedAnswer() =
runTest {
// 7363
addNoteUsingBasicTypedModel("Hello", "World")
addBasicTypedNoteType("Hello", "World")
Copy link
Member

Choose a reason for hiding this comment

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

addBasic[Note]

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'm sorry but I can't guess what change you require here. This is not code I can copy paste at least

Copy link
Member

Choose a reason for hiding this comment

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

Rename the method to either addBasic or addBasicNote

The current proposal is verbose, which detracts from readability

AnkiDroid/src/test/java/com/ichi2/anki/AbstractFlashcardViewerTest.kt
@@ -256,8 +256,8 @@ class AbstractFlashcardViewerTest : RobolectricTest() {

LanguageHintService.setLanguageHintForField(col.notetypes, withLanguage, typedField, Locale("ja"))

addNoteUsingModelName(withLanguage.getString("name"), "ichi", "ni")
addNoteUsingModelName(normal.getString("name"), "one", "two")
addNoteUsingNoteTypeName(withLanguage.getString("name"), "ichi", "ni")
Copy link
Member

Choose a reason for hiding this comment

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

ignorable: name feels too long. No immediate suggestions

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 agree. But the note type "Basic (Type in the answer)" is quite hard to rephrase

AnkiDroid/src/test/java/com/ichi2/anki/CardBrowserTest.kt Outdated
addNoteUsingBasicAndReversedModel("Hello", "World")
addNoteUsingBasicAndReversedModel("Hello", "Anki")
addBasicAndReversedNoteType("Hello", "World")
addBasicAndReversedNoteType("Hello", "Anki")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
addBasicAndReversedNoteType("Hello", "Anki")
addBasicAndReversed("Hello", "Anki")

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 think it should be "addBasicAndReversedNote". I agree the "type" is too much. It's still nice to be explicitly state its a note that is added

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 also removed the "type" at the end of every other changed functions

Copy link
Member

Choose a reason for hiding this comment

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

Sure

AnkiDroid/src/test/java/com/ichi2/anki/CardBrowserTest.kt Outdated Show resolved Hide resolved
AnkiDroid/src/test/java/com/ichi2/testutils/TestClass.kt Outdated Show resolved Hide resolved
@david-allison david-allison added Needs Second Approval Has one approval, one more approval to merge cleanup Non functional change that would improve the code readability labels Jan 3, 2025
mikehardy
mikehardy approved these changes Jan 4, 2025
View reviewed changes
Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

let's go

@mikehardy mikehardy added this pull request to the merge queue Jan 4, 2025
Merged via the queue into ankidroid:main with commit 1cdb1f3 Jan 4, 2025
9 checks passed
@github-actions github-actions bot removed the Needs Second Approval Has one approval, one more approval to merge label Jan 4, 2025
@github-actions github-actions bot added this to the 2.21 release milestone Jan 4, 2025
@Arthur-Milchior Arthur-Milchior deleted the model_3 branch January 4, 2025 18:58
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

@mikehardy mikehardy mikehardy 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
@Arthur-Milchior @mikehardy @david-allison