Skip to content
This repository has been archived by the owner on Dec 31, 2023. It is now read-only.

Add getNoteTags, updateNoteTags and updateNote #370

Merged
merged 1 commit into from
Apr 1, 2023

Conversation

introt
Copy link
Contributor

@introt introt commented Dec 30, 2022

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

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 FooSoft#369
@introt
Copy link
Contributor Author

introt commented Dec 30, 2022

@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
Copy link
Contributor

Aquafina-water-bottle commented Dec 31, 2022

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
Copy link
Contributor Author

introt commented Dec 31, 2022

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 desired1.

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"
    ],
    ...
}

Footnotes

  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
Copy link
Contributor

Aquafina-water-bottle commented Jan 3, 2023

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
Copy link
Owner

FooSoft commented Jan 24, 2023

@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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants