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

Parse nodeIDs #92

Merged
merged 2 commits into from
Feb 22, 2023
Merged

Parse nodeIDs #92

merged 2 commits into from
Feb 22, 2023

Conversation

benjamin-weller
Copy link
Contributor

This change allows for a parsing of note IDs. This is the first step in allowing (eventually) the automatic storing of the created note ID in the markdown file.

@jasonwilliams
Copy link
Owner

@benjamin-weller could you explain where these noteIDs come from? Why are they added and what is generating them, the markdown would be hand written most of the time. I may be missing some context here

@benjamin-weller
Copy link
Contributor Author

benjamin-weller commented Dec 26, 2022

These note IDs are uniquely generated by Anki.
image

See the note/card ID section of the browser search documentation here.

IIUC these IDs uniquely identify the note in question; as such you could eventually update a specific note given its parsed note ID.

How I intend to eventually allow for updating of already created notes:

  • Parse noteId out of the comments (this change)
  • When creating the deck/uploading the cards to Anki via AnkiConnect simply check and see if a note ID is set when we parsed the card from markdown. If the ID is set, and isn't some sentinel value, then let's call AnkiConnect to update the card instead of trying to create it: image

Now that there's a way for us to update notes, I would like to have a way for us to automatically insert the note IDs into the markdown during the deck/note creation command.

How I intend to get/insert the note ID into markdown:

  • Call Anki: Send To Deck (or other creation command) in VS Code
  • This creation command will eventually call AnkiConnect to create the notes via addNotes:
    image
  • Following the creation, AnkiConnect will respond to us with the note IDs of the created notes, I would then insert these IDs into the markdown via the edit API

@jasonwilliams
Copy link
Owner

jasonwilliams commented Jan 5, 2023

Hey @benjamin-weller great work on making a start on this, I wasn't ignoring, it was just the holidays and I didn't have time to look over this. Hopefully I should have a bit more time now.

Thinking about the workflow here...
Someone will create a note in VS Code, when they send it to Anki, AnkiConnect gives us an ID back in the response I believe. Are you planning to take the ID and insert it back into the markdown once sent? (I understand this PR doesn't deal with that right now)

Do you have an idea how this comment will look like?

@benjamin-weller
Copy link
Contributor Author

benjamin-weller commented Jan 10, 2023

100% no worries, you've been very responsive before and honestly I have no expectations around super prompt responses, this is OSS; not much money changing hands here.

Someone will create a note in VS Code, when they send it to Anki, AnkiConnect gives us an ID back in the response I believe. Are you planning to take the ID and insert it back into the markdown once sent? (I understand this PR doesn't deal with that right now)

Yes, that's the ultimate goal

Do you have an idea how this comment will look like?

Yes the comment should look like <!-- notecardId: ###### --> or <!-- notecardId = ###### -->. I've generally tried to be tolerant of spaces in my regex.

"anki.md.card.notecardIdPattern": {
"type": "string",
"default": "<!--\\s*?notecardId\\s*?\\[:=\\]\\s*?(\\d+)\\s*?-->",
"description": "Text to match match the commented notecard ID."
Copy link
Owner

Choose a reason for hiding this comment

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

Do we need this to be configurable? I have a feeling if people change this it could become messy.
I'd start off with it being hardcoded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, yes I guess it could result in lots of questions/mis-configured regexes (and thus lots of github issues).

I'm going making this value just exist in code, I'll add it to a constants file if there is one, or just leave it by the parsing logic if there isn't.

@jasonwilliams
Copy link
Owner

Hey @benjamin-weller, maybe im doing something wrong here but could you explain why noteId isn't coming through when I test it?

noNoteId

@jasonwilliams
Copy link
Owner

jasonwilliams commented Jan 13, 2023

I've also realised this repo doesn't support proper linting on CI so im going to sort that out. I will PR it, you may need to rebase apologies in advance

@benjamin-weller
Copy link
Contributor Author

Sounds good I'll rebase here.

The unit tests pass and that's what I tested against, can you give me a direction as to how to set up your testing environment?

Is it like building a trial/test version of the extension and installing that?

@jasonwilliams
Copy link
Owner

Sure

https://github.com/jasonwilliams/anki/blob/main/CONTRIBUTING.md#debugging
I clicked "Run & Debug" icon on the left, then clicked the play button next to "Run Extension".

This should launch another instance of VSCode, I used that to test. In the original instance you should be able to set breakpoints and inspect the code

@benjamin-weller
Copy link
Contributor Author

Preliminary investigation is leading me to confusing places:
image

I'll continue to debug.

@benjamin-weller
Copy link
Contributor Author

benjamin-weller commented Feb 7, 2023

I don't know why the regex wasn't working with my previous setup, but I manually tested, and ran the unit tests; both now pass.

Here's what I get during manual debugging:
image

@jasonwilliams
Copy link
Owner

Awesome work thanks, I’ll take another look. Also it seems there’s a conflict with package.json, you may need to try and rebase

@jasonwilliams
Copy link
Owner

Your lockfile causes conflicts, like its trying to set it back to version 1, what version of nodejs/npm are you using? Everything else seems ok

@benjamin-weller
Copy link
Contributor Author

Ok, I'm relatively new to the git flow, please let me know if my changes here match up and can be merged.

@jasonwilliams
Copy link
Owner

LGTM

@jasonwilliams jasonwilliams merged commit f618f95 into jasonwilliams:main Feb 22, 2023
@jasonwilliams
Copy link
Owner

Hey @benjamin-weller now this is merged what are your next steps?

@benjamin-weller
Copy link
Contributor Author

I've outlined what I intend to do next here.

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

Successfully merging this pull request may close these issues.

None yet

2 participants