Send updates when NoteID is populated #106

benjamin-weller · 2023-03-02T00:20:03Z

Jason can you please take a look at generally what I'm thinking about doing and give me a LGTM or not. I just don't want to get going on these changes and be barking up the wrong tree.

Continuation of PR 92.

jasonwilliams · 2023-03-15T12:04:46Z

Hey @benjamin-weller sorry for being slow to look at this. I've been away i will try and take a look this week

benjamin-weller · 2023-03-15T22:09:04Z

SG, no worries. I'm relatively green WRT to JavaScript async + await, so I think that's likely where I could have missed something.

Additionally how would you suggest I write unit tests for these changes?

jasonwilliams · 2023-03-20T21:42:42Z

src/models/Deck.ts

+  async createAndUpdateCards() {
+    // Push the updated cards (based on noteID)
+    const updateCards = this.cards.filter((v) => v.noteId);
+    this._pushUpdatedCardsToAnki(updateCards);


You could probably use reduce so you only cycle through the cards once

this.cards.reduce((acc, v) => {v.noteId ? acc[update].push(v) : acc[new].push(v) }, {})

Or something like that

I'll going to use forEach here because that's what makes sense in my brain.

jasonwilliams · 2023-03-20T21:46:09Z

src/models/Deck.ts

-  async pushNewCardsToAnki() {
+  // Calls anki to update the fields of all the passed cards.
+  private async _pushUpdatedCardsToAnki(cards: Card[]) {
+    cards?.map((card) => this.ankiService?.updateFields(card));


This method is async but there's no await being used.
You're also not returning any value so map is redundant here, you may instead just want to use forEach.

Do you want to wait for this function to finish? If so you could return the promises into a Promise.all() and await that

Suggested change

cards?.map((card) => this.ankiService?.updateFields(card));

const cardsPromise = cards?.map((card) => this.ankiService?.updateFields(card));

await Promise.all(cardsPromise);

The ? on cards would be redundant as it should always be an array coming into this function.

I'm going to drop the async on this function as I don't care exactly when this finishes.
I'll also switch to forEach.
I'll remove the optional chaining you mentioned.

I think we do need to care when operations like this finish though. Due to the progress bar that’s shown in VSCode, it needs to know when to stop showing.

I've taken your advice and ensured I'm awaiting the async _pushUpdatedCardsToAnki function now.

jasonwilliams · 2023-03-20T21:51:45Z

Yeah so far looks good.
Just so I understand, we don't have the code that handles adding the noteID into the markdown just yet right?

So far I see support for parsing the note and updating/instead of pushing if there is a noteid (this PR)

For tests:
I would say do the best you can, but don't worry too much about it. Im happy to help out on tests, I may try and migrate to vitest soon anyway so if you're not up to it I can change and add later.

benjamin-weller · 2023-03-25T04:06:52Z

Just so I understand, we don't have the code that handles adding the noteID into the markdown just yet right?

Yes I have some "working" code for this on my computer. I want to roll this change out first because I perceive that change to be higher risk/would clog up the review for this change.

WRT tests, I manually tested. Idk how exactly I would mock updating a card in Anki here? Unless you have a good idea now on how to do that I'm leaning towards not implementing them for this feature. That won't be the case for the next one (updating the markdown) as that's higher risk imo.

Proof of manual testing (works on my machine):

Deck Prior to run:

Content of MD file:

# Deck Name Is Definately Not Scratch

## NEW NAME IS DEFINATELY NOT YOLO

<!-- notecardId: 1679716453114 -->

New back side of card

Ran: Send To Own Deck

Deck post run:

I want to point out that my changes allow you to update a notecard regardless of which deck it exists in. This may feel like a bug at first, but it's what I want, my use case; I may relocate an entire subsection of markdown document, but that doesn't mean when I re-push to Anki that I want to re-create new cards, I just want to know that the particular cards is up to date.

jasonwilliams · 2023-03-25T06:46:49Z

I want to point out that my changes allow you to update a notecard regardless of which deck it exists in. This may feel like a bug at first, but it's what I want, my use case; I may relocate an entire subsection of markdown document, but that doesn't mean when I re-push to Anki that I want to re-create new cards, I just want to know that the particular cards is up to date.

sure this sounds fair, but it may conflict with the “Send dir to Anki” command. Which sends cards to Anki based on their directory location. If I have noteids on the cards and move them which operation wins out?

We may want to think about and how we respond to it

mkraenz · 2023-03-25T17:36:18Z

Hi, awesome job! Updates are the one feature I've been missing, so very happy this is getting tackled. Thank you so much!

In eager awaiting of this, I've written a script to update an existing markdown file (without note ids) with the note ids from Anki, in a similar format as #106 (comment) does (though I'm using  for now).

You can find it in this gist https://gist.github.com/mkraenz/a7e0473ca80c07522ff75dc2fd9e52ba

Maybe it becomes a reference for you or somebody else. In particular, the sanitization of the card front is not trivial.
Note that I do assume a nicely linted markdown file.

The summary at the end looks like this for 526 cards with backticks, question marks, parentheses in the card front all being gracefully handled. Only one of the cards cannot be resolved to its note id.

@@@@@@@@@@@@@@@@@@@@@@@ DONE @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
OVERWRITTEN FILE ON DISK:  /home/mirco/Desktop/anki.test.md.withNoteids.md 

Total number of cards processed:  526
Number of successfully written note ids for cards:  525
Number of missing node ids for cards:  1
MISSING NOTE_IDS FOR CARDS:
 linux `$RANDOM`

PS: Sorry for randomly dropping this into this PR.

benjamin-weller · 2023-03-30T23:46:02Z

I want to point out that my changes allow you to update a notecard regardless of which deck it exists in. This may feel like a bug at first, but it's what I want, my use case; I may relocate an entire subsection of markdown document, but that doesn't mean when I re-push to Anki that I want to re-create new cards, I just want to know that the particular cards is up to date.

sure this sounds fair, but it may conflict with the “Send dir to Anki” command. Which sends cards to Anki based on their directory location. If I have noteids on the cards and move them which operation wins out?

We may want to think about and how we respond to it

I'm not certain exactly which one wins out tbh. I'm willing to feature flag this (only have cards be updated if the user opts in), this ensures that they explicitly agree that things might not be 100% compatible. Is that fair for you?

benjamin-weller · 2023-03-31T00:25:39Z

Hi, awesome job! Updates are the one feature I've been missing, so very happy this is getting tackled. Thank you so much!

In eager awaiting of this, I've written a script to update an existing markdown file (without note ids) with the note ids from Anki, in a similar format as #106 (comment) does (though I'm using  for now).

You can find it in this gist https://gist.github.com/mkraenz/a7e0473ca80c07522ff75dc2fd9e52ba

Maybe it becomes a reference for you or somebody else. In particular, the sanitization of the card front is not trivial. Note that I do assume a nicely linted markdown file.

The summary at the end looks like this for 526 cards with backticks, question marks, parentheses in the card front all being gracefully handled. Only one of the cards cannot be resolved to its note id.

@mkraenz It's great to hear that hopefully this feature will have users!

Yes the card sanitation is currently the most tricky part, I assume that the text of the header is equivalent to the text of the front of the card (I ask the HTML for the inner content but even then there can be stuff like <strong> tags which interfere with string matching). I'll take a closer look at your implementation, I'm really hoping that I can find a NPM package that will just simply reverse engineer a given HTML string to markdown.

jasonwilliams · 2023-04-02T20:19:57Z

I'm not certain exactly which one wins out tbh. I'm willing to feature flag this (only have cards be updated if the user opts in), this ensures that they explicitly agree that things might not be 100% compatible. Is that fair for you?

Ok lets feature flag it for now.
I also think at some point in the future we could have a command that re-moves cards based on their new position in the directory (assuming you're using the send to dir feature). This would mitigate the issue we face of moving cards then re-updating to somewhere else.

src/AnkiService.ts

benjamin-weller · 2023-04-14T19:36:27Z

Is there anything else I need to do such that these changes are ready to merge?

jasonwilliams

Sorry I’ve been slow on this, LGTM

jasonwilliams · 2023-05-03T08:43:46Z

@benjamin-weller how has this been for you? Have you been testing it?
I guess we need a commit which adds the noteID back into the markdown

benjamin-weller · 2023-05-04T04:07:23Z

Hey, I've created a new PR, let's talk there.

Spirarel · 2023-05-09T04:10:07Z

I'm not certain exactly which one wins out tbh. I'm willing to feature flag this (only have cards be updated if the user opts in), this ensures that they explicitly agree that things might not be 100% compatible. Is that fair for you?

Ok lets feature flag it for now. I also think at some point in the future we could have a command that re-moves cards based on their new position in the directory (assuming you're using the send to dir feature). This would mitigate the issue we face of moving cards then re-updating to somewhere else.

Are you guys approaching a release soon or do we need to install from main?

Sketch out what my changes would be

52693f8

benjamin-weller mentioned this pull request Mar 2, 2023

Parse nodeIDs #92

Merged

benjamin-weller added 2 commits March 3, 2023 21:01

POC for update cards

30bd94f

Cleaned up the code

a569bd2

benjamin-weller changed the title ~~Send updates when NoteID is populated, and Insert NoteID into markdown when creating a card~~ Send updates when NoteID is populated Mar 15, 2023

jasonwilliams reviewed Mar 20, 2023

View reviewed changes

benjamin-weller added 2 commits March 24, 2023 21:07

Addressed comments

8209d10

Merge branch 'main' of https://github.com/benjamin-weller/anki

992ea5f

benjamin-weller added 2 commits March 30, 2023 17:13

Added feature flag for updates, awaited async functions

01cc062

Merge branch 'main' of https://github.com/benjamin-weller/anki

7289584

jasonwilliams reviewed Apr 2, 2023

View reviewed changes

src/AnkiService.ts Outdated Show resolved Hide resolved

benjamin-weller added 2 commits April 2, 2023 15:33

Clean up, remove logging

30c1299

Merge branch 'main' of https://github.com/benjamin-weller/anki

bb56d5d

benjamin-weller mentioned this pull request Apr 14, 2023

Cards not updated after changing .md file and Send To Own Deck #103

Open

jasonwilliams approved these changes Apr 14, 2023

View reviewed changes

jasonwilliams merged commit 6824b5b into jasonwilliams:main Apr 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send updates when NoteID is populated #106

Send updates when NoteID is populated #106

benjamin-weller commented Mar 2, 2023 •

edited

Loading

jasonwilliams commented Mar 15, 2023

benjamin-weller commented Mar 15, 2023

jasonwilliams Mar 20, 2023 •

edited

Loading

benjamin-weller Mar 25, 2023 •

edited

Loading

jasonwilliams Mar 20, 2023 •

edited

Loading

benjamin-weller Mar 25, 2023

jasonwilliams Mar 25, 2023

benjamin-weller Mar 31, 2023

jasonwilliams commented Mar 20, 2023

benjamin-weller commented Mar 25, 2023

jasonwilliams commented Mar 25, 2023 •

edited

Loading

mkraenz commented Mar 25, 2023 •

edited

Loading

benjamin-weller commented Mar 30, 2023 •

edited

Loading

benjamin-weller commented Mar 31, 2023

jasonwilliams commented Apr 2, 2023

benjamin-weller commented Apr 14, 2023

jasonwilliams left a comment

jasonwilliams commented May 3, 2023

benjamin-weller commented May 4, 2023

Spirarel commented May 9, 2023

	cards?.map((card) => this.ankiService?.updateFields(card));
	const cardsPromise = cards?.map((card) => this.ankiService?.updateFields(card));
	await Promise.all(cardsPromise);

Send updates when NoteID is populated #106

Send updates when NoteID is populated #106

Conversation

benjamin-weller commented Mar 2, 2023 • edited Loading

jasonwilliams commented Mar 15, 2023

benjamin-weller commented Mar 15, 2023

jasonwilliams Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

benjamin-weller Mar 25, 2023 • edited Loading

Choose a reason for hiding this comment

jasonwilliams Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

benjamin-weller Mar 25, 2023

Choose a reason for hiding this comment

jasonwilliams Mar 25, 2023

Choose a reason for hiding this comment

benjamin-weller Mar 31, 2023

Choose a reason for hiding this comment

jasonwilliams commented Mar 20, 2023

benjamin-weller commented Mar 25, 2023

jasonwilliams commented Mar 25, 2023 • edited Loading

mkraenz commented Mar 25, 2023 • edited Loading

benjamin-weller commented Mar 30, 2023 • edited Loading

benjamin-weller commented Mar 31, 2023

jasonwilliams commented Apr 2, 2023

benjamin-weller commented Apr 14, 2023

jasonwilliams left a comment

Choose a reason for hiding this comment

jasonwilliams commented May 3, 2023

benjamin-weller commented May 4, 2023

Spirarel commented May 9, 2023

benjamin-weller commented Mar 2, 2023 •

edited

Loading

jasonwilliams Mar 20, 2023 •

edited

Loading

benjamin-weller Mar 25, 2023 •

edited

Loading

jasonwilliams Mar 20, 2023 •

edited

Loading

jasonwilliams commented Mar 25, 2023 •

edited

Loading

mkraenz commented Mar 25, 2023 •

edited

Loading

benjamin-weller commented Mar 30, 2023 •

edited

Loading