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

Implemented some libanki TODOs #17442

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RobozinhoD
Copy link
Contributor

Purpose / Description

I tried to find some TODO in the project and fixed these in libanki. I hope it helps

Approach

Followed upstream's implementation

https://github.com/ankitects/anki/blob/edf59c2bb2a3dad36115af7518cdcbb3ee397089/pylib/anki/decks.py

How Has This Been Tested?

I don't know how to test it

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

@RobozinhoD RobozinhoD changed the title Libdecks Fixed some libanki TODOs Nov 14, 2024
@RobozinhoD RobozinhoD changed the title Fixed some libanki TODOs Implemented some libanki TODOs Nov 14, 2024
@BrayanDSO BrayanDSO added the Needs Second Approval Has one approval, one more approval to merge label Nov 14, 2024
private fun addDeck(deck: Deck): OpChangesWithId {
TODO()
@Suppress("unused")
fun addDeck(deck: anki.decks.Deck): OpChangesWithId {
Copy link
Member

Choose a reason for hiding this comment

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

This is the only somewhat questionable one:

Do we want to continue using Deck, or move to the upstream Protobuf

For now, this is the easiest course of action

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that this one should be equal to libanki and that we can create another util or extension method for Deck

@david-allison david-allison added squash-merge The pull request currently requires maintainers to "Squash Merge" Needs Author Reply Waiting for a reply from the original author 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 Nov 16, 2024
@david-allison
Copy link
Member

@RobozinhoD Thanks! Could you squash this and it'll be good to go

libanki updateDict()

libanki update()

libanki have()

libanki addNormalDeckWithName()

libanki addDeck()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Author Reply Waiting for a reply from the original author Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) squash-merge The pull request currently requires maintainers to "Squash Merge"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants