FooSoft / anki-connect

Anki plugin to expose a remote API for creating flash cards.
https://foosoft.net/projects/anki-connect/
Other
1.92k stars 218 forks source link

Add getNoteTags, updateNoteTags and updateNote #370

Closed introt closed 1 year ago

introt commented 1 year ago

getNoteTags returns [str ...] of tags for a given note (note: int)

updateNoteTags removes the existing tags and sets new tags

updateNote combines the old updateNoteFields and the new updateNoteTags

Closes #369

introt commented 1 year ago

@FooSoft Ready for review, thanks for your feedback! Besides the docs and the tests, I made updateNote work with either tags or fields and fail when neither is provided. Doing multiple things does mean that an earlier one failing stops the latter things from happening - tried to reflect this in the docs.

Hierarchical tags like::this::for::example seem to sometimes cause trouble, but since none of the tests seem to deal with those elsewhere I've left those cases untested.

Aquafina-water-bottle commented 1 year ago

Hey @introt, I'm not Foosoft but I'm going to take a wild guess and say that updateNote will be rejected because the multi option already exists. A similar PR (#262) was closed for that reason.

On a side note, I also had to scrap a bit of work myself since I didn't realize multi existed until finding that exact PR I linked earlier. I'm wondering if it can be clearer in the documentation that it exists?

introt commented 1 year ago

Hi @Aquafina-water-bottle, thanks for your comment/review. I agree that the current implementation isn't optimal - a multi of updateNoteFields and updateNoteTags handles errors better - but I disagree that updateNote as a whole should not exist.

Imo the ideal API would have a "canonical" json representation of a note which both addNote and updateNote would accept. This implementation also fails to update the deck, which might be desired[^1].

Unlike a multi of updateNoteFields, updateNoteTags, and changeDeck, updateNote should return a single null as error if and only if everything goes as planned. This way the simplest Anki-Connect client doesn't need to implement multi result parsing, and can simply dump all the errors as a string (representation of a json list) on failure.

I'd even go as far as implement a uploadNote or setNote, which could, according to options, re-add a card deleted from Anki, along with getNote, which, unlike notesInfo, would include the deckName and the fields as a list of strings, conforming to the "canonical" note representation. (Python's dictionaries are ordered by default nowadays, and clients without ordered dicts can fetch the ordering via modelFieldNames.)

Looking forward to hearing your opinions on these!

{
    "id": 1514547547030
    "deckName": "Default",
    "modelName": "Basic",
    "fields": {
        "Front": "front content",
        "Back": "back content"
    },      
    "tags": [
        "yomichan"
    ],
    ...
}

[^1]: Side note: Is there a preferable way to handle "per-occasion" (filtered) decks from outside Anki? Should the notes' deck be changed or the notes tagged to be picked up by an existing filered deck? Is setSpecificValueOfCard useful here? I've just gone with tags and manually edited filtered decks, can those be created with Anki-Connect? No relevant mentions in the README.

Aquafina-water-bottle commented 1 year ago

Hey @introt,

To clarify, I personally am not very opinionated on whether something like that actually exists in the API or not, and at the end of the day, it's @FooSoft's call to make anyways. I definitely do see is uses though, and thanks for the super detailed reply!

FooSoft commented 1 year ago

@introt apologies for the delayed review, had to deal with IRL things.

I like your changes, I believe that a certain amount of redundancy is fine in this case. AnkiConnect API isn't the paragon of beauty and has grown "organically" over the years. I think updateNote is nice and clean :)

There are some merge conflicts, if you could fix them I will get this merged in ASAP.