Added new option allowing for multiline cards with empty lines

[alberti42]

Added multilineCardEndMarker setting storing a delimiting string. The user can choose for example '---'. This allows handling flashcards with empty lines, without resorting to expedients such as hidden characters to mimic the empty line.

[ronzulu]

Hi @alberti42 thanks for this, many people have been after this for a long time, and will be very happy when it is released.

A few requests:

1. Currently some of the existing unit test cases fail. Please update your enhancement so that the existing tests all pass. See https://www.stephenmwangi.com/obsidian-spaced-repetition/contributing/#code for details
2. Update the user facing documentation, see https://www.stephenmwangi.com/obsidian-spaced-repetition/contributing/#documentation
3. Let me know and I'll review at that time
4. We will need some new unit test cases. You may prefer to wait till I complete step 3. and provide feedback about functionality.

Thanks once again!
Cheers

[ronzulu]

Hi @alberti42, I've just noticed that you have rearchitected the parser from the ground up. You must have put in much time and thought into this!

@4Source initiated the discussion Rework the flashcard seperator/detecting system #1018 noting that there are many outstanding requests that relate to the parser and that it's not very easy to enhance in its current form.
(The list including the issue titles at https://github.com/users/ronzulu/projects/1)

So your rearchitect of this part of the plugin might be perfect!

I don't have familiarity with https://github.com/pegjs/pegjs/wiki, one question that comes to mind is regarding tokens that you currently have hardcoded in grammar.pegjs. Will you be able to modify this to use the settings contained in:

@4Source and @st3v3nmw given the significance of the changes in this PR, would be great if you could both look at this as well.

Cheers
Ronny

[alberti42]

Hi @alberti42 thanks for this, many people have been after this for a long time, and will be very happy when it is released.

A few requests:

1. Currently some of the existing unit test cases fail. Please update your enhancement so that the existing tests all pass. See https://www.stephenmwangi.com/obsidian-spaced-repetition/contributing/#code for details

2. Update the user facing documentation, see https://www.stephenmwangi.com/obsidian-spaced-repetition/contributing/#documentation

3. Let me know and I'll review at that time

4. We will need some new unit test cases. You may prefer to wait till I complete step 3. and provide feedback about functionality.

Thanks once again! Cheers

It required much more work than what I thought because I wanted to have a clean solution. I created a true parser. It is quick and easy to maintain for the future. The old parser was quite untransparent and could not be easily extended.

The new parser makes use of Peggy. The rules are defined with peggy.pegjs. You can find highlighter to supper in your editor highlighting of peg.js syntax.

If you modify peggy.pegjs, you need to run npm run grammar to generate automatically peggy.mjs file, which is the real parser.

The peggy.pegjs can be tested with https://peggyjs.org/online.html, which is great when you want to test the different grammar rules.

The only part that is not nice in peggy.pegjs is the block called HACK. You will see it. These are a bunch of functions adapted from the automatically generated peggy.mjs files. The purpose is to allow the user-defined keywords, when the user wants to redefine "??" or "?".

For the multilineCardEndMarker separating multiline notes, I like to use '---'.

If you leave the field empty in Obsidian configuration pane, then the old behavior will apply.

PS: All tests are now passed. I also translated the label in the few languages.. although the translation is not great yet.

[alberti42]

Hi @alberti42, I've just noticed that you have rearchitected the parser from the ground up. You must have put in much time and thought into this!

@4Source initiated the discussion Rework the flashcard seperator/detecting system #1018 noting that there are many outstanding requests that relate to the parser and that it's not very easy to enhance in its current form. (The list including the issue titles at https://github.com/users/ronzulu/projects/1)

So your rearchitect of this part of the plugin might be perfect!

I don't have familiarity with https://github.com/pegjs/pegjs/wiki, one question that comes to mind is regarding tokens that you currently have hardcoded in grammar.pegjs. Will you be able to modify this to use the settings contained in:

@4Source and @st3v3nmw given the significance of the changes in this PR, would be great if you could both look at this as well.

Cheers Ronny

Sorry, I did not see this message when I wrote the previous one just a moment ago.

Yes, we have a new parser, which should be much easier to understand and maintain. The rules can still be simplified a tiny bit to make the grammar more streamlined.. but let's first see what you think of it.

I don't have familiarity with https://github.com/pegjs/pegjs/wiki, one question that comes to mind is regarding tokens that you currently have hardcoded in grammar.pegjs. Will you be able to modify this to use the settings contained in:

It now works with the user-based keywords. For this, I required what I called a HACK in the previous message. The HACK is pretty straightforward and does not compromise efficiency. At least, it does not make it slower. However, cosmetically, it does not look nice if you are a purist of good coding.

[4Source]

I can' say much about the parser it self haven't investigated in it jet. But what I have seen/understand looks great to me for future adjustments and planed features from the discussion

[4Source]

For the multilineCardEndMarker separating multiline notes, I like to use '---'.

I think this is a bad Idea we had already issues with the --- because it is used in other formatting like tables or horizontal lines

If you modify peggy.pegjs, you need to run npm run grammar to generate automatically peggy.mjs file, which is the real parser.

If I understand this correctly I think how this works should be well documented somewhere in dev guide

[alberti42]

For the multilineCardEndMarker separating multiline notes, I like to use '---'.

I think this is a bad Idea we had already issues with the --- because it is used in other formatting like tables or horizontal lines

Sure. I did not say that this needs to be the default behavior. This is just what I find nice for me. From the preference pane, the user can choose --- or +++ or any other sequence of characters she/he likes.

If you modify peggy.pegjs, you need to run npm run grammar to generate automatically peggy.mjs file, which is the real parser.

If I understand this correctly I think how this works should be well documented somewhere in dev guide

Yes. We should add one line to explain this step.

[4Source]

Sure. I did not say that this needs to be the default behavior. This is just what I find nice for me. From the preference pane, the user can choose --- or +++ or any other sequence of characters she/he likes.

I just meant this should not be the default setting. So user who try this plugin for first time or user there not investigate in the settings have a normally functional experience

[alberti42]

Sure. I did not say that this needs to be the default behavior. This is just what I find nice for me. From the preference pane, the user can choose --- or +++ or any other sequence of characters she/he likes.

I just meant this should not be the default setting. So user who try this plugin for first time or user there not investigate in the settings have a normally functional experience

I agree 100%. Actually, I would have left the field as empty as the default setting. When empty, the old behavior applies, which is what probably people in the community expect.

[ronzulu]

That's great!

@alberti42 , there may be an alternative to the HACK. Have you come across this? Does this help?
Dynamically generate / parse rules based on configuration #512

Cheers
Ronny

[alberti42]

That's great!

@alberti42 , there may be an alternative to the HACK. Have you come across this? Does this help? Dynamically generate / parse rules based on configuration #512

Cheers Ronny

Hey Ronny,

Thanks! Yes, I had read the page before. You can indeed use predicates & { predicate } to evaluate whether an expression finally matches or not. This is also nicely documented in https://peggyjs.org/documentation.html.

I have used such predicates already for evaluating dynamically the flags deciding what sybols define a close. They work nicely.

The reason why I did not use it for the other symbols such as ? and ?? is because I thought the parsing efficiency may be affected. But we can give it a try.

To try this, we would need to define our own grammar of what are the sequences of characters the user is allowed to type. In principle, we could decide any characters except newlines. However, my guess is that the predication function will be evaluated a lot of times to decide if there is a match or not.

There are also other options if you do not like the HACK (I don't like it either).

We could generate the parser.mjs anew directly in Obsidian every time the user changes the configuration in the setting pnae. This happens only rarely anyway, and peggyjs is quite fast to generate from the grammar.pegjs a new .mjs file. The downside is that we need to ship with the app the entire transpiler of peggy.js. So, I would avoid it if we find other and better solutions.

The other possibility is that we use in the grammar.pegjs different PLACEHOLDERSTRINGS123 for each of the strings to be then replaced by the user.

We then write a little .js program of a few lines introducing a second-step transpiler. The purpos is to process the parser.mjs generated by peggyjs. This step can be combined with npm run grammar. So it will look like as a single step for the developer of other features.

Such second-step transpiler replaces the static placeholders with javascript code importing the values for each placeholder from the variable options. In fact, you may have noticed that it is already possible to dynamically provide content to peggyjs parser, and we are already using it.

This last approach is possibly the most elegant, but not without caveats. If we use static placeholders, peggyjs optimizes the parsing by detecting their string length. Our transpiler would have to replace in the .mjs code the static length with one dynamically optained from the user variables. This is quite efficient in terms of execution speed. However, the transpiler is a bit fragile because it needs to do a surgery on the .mjs file about the length of the PLACEHOLDERS. If something changes in future versions of peggyjs, these changes may break the compatibility with our transpiler, and some small fix will be needed.

Other possibilities. I know it is possible to create plugins for peggyjs, but I do not know if these allow to have dynamical keywords.

And the last possibility, we improve peggyjs and make a pull request to introduce the feature of dynamical keywords. 😅

Bottom line:

If you are happy with the parser, I would focus on testing all features, removing any possible small bugs, and streamlining the grammar file. It can be cleaned up a little. We can then release with the community.

We could think how to replace the HACK in the future. In my assessment, the HACK has no impact on the execution speed or anything else. It is not elegant, and it is not easy to understand someone just opens the code for the first time, and was not given any explanation.

Best,
Andrea

[ronzulu]

Hi @alberti42 thanks for such a comprehensive post!

If I understand correctly, the best way would be for the parser to be generated whenever any of the relevant user options are changed. The downside is needing to include the peggy package (and it's dependencies) increasing memory requirements.

Out of curiosity, I tried the following code which dynamically generates the parser:

test("hello", () => {
    var peg = require("peggy");
    var inlineMark = '"::" / "--"';
    var grammar = `
start 
    = q:question inline_mark a:answer { return "Q: " + q +  ", A: " + a}

question = text

answer = text

text = t:[a-zA-Z ]+ { return t.join(""); }

inline_mark = ${inlineMark}
`
    var parser = peg.generate(grammar);

    var actual = parser.parse("What should I eat for dinner::chocolate cake");
    expect(actual).toEqual("Q: What should I eat for dinner, A: chocolate cake");

    actual = parser.parse("What should I eat for dinner--chocolate cake");
    expect(actual).toEqual("Q: What should I eat for dinner, A: chocolate cake");
});

I think this is a very clean solution. In terms of package size, I came across this web site:
https://bundlephobia.com/package/peggy@4.0.3

It reported:

To me, the benefit outweighs the additional memory usage.

I'm wondering if you have had a chance to look at some of the other related PRs and issues:
https://github.com/users/ronzulu/projects/1

With some of those, there will be more user options, and so whatever we come up with needs to handle the future requirements.

Cheers
Ronny

[alberti42]

I tested your example and worked very nicely. Great that it is so smooth to generate the parser from the grammar.

var parser = peg.generate(grammar);

I agree, speed and memory consumption should not be a problem. Even if this operation takes a bunch of ms, it is only executed rarely, when the user changes the keywords in the configuration pane.

I would only add a debouncing solution, so the function is not called to many times, each time that the user press a key in the editing field; see for example https://github.com/alberti42/obsidian-plugins-annotations/blob/main/src/main.ts#L566

I'm wondering if you have had a chance to look at some of the other related PRs and issues: https://github.com/users/ronzulu/projects/1

Not really. Is there anything in particular you think it is important to consider? I think the parser is very powerful and I cannot imagine a situation that cannot be handled.

I was surprised that currently all parsed information is not really stored. At the end, you only store the beginning and end position of the card. In principle, one can make more use of the parser and store the different parts of a flash card in different fields, with annotated the position in the text. But, it seems you did not need this extra structuring of parsed information... so maybe we do not need to put extra work on something like this that is not really needed.

[ragrawal]

Any idea when this PR will be merged ?

[ronzulu]

I tested your example and worked very nicely. Great that it is so smooth to generate the parser from the grammar.

That all sounds great. Are you planning to update your code to use this technique?

Not really. Is there anything in particular you think it is important to consider? I think the parser is very powerful and I cannot imagine a situation that cannot be handled.

I'm sure you're right, so let's not worry about future changes right now.

Thanks!

[alberti42]

That all sounds great. Are you planning to update your code to use this technique?

I have implemented it. It worked for my usage cases. There are some tests that did not pass yet. I will have to look at them in the next days to understand what goes wrong. Hopefully, it will be a very quick fix.

I tested the time to generate the parser. It is already in the debouncing function, which avoids generating the parser too many times if the user changes the options very quickly.

In any case, it takes on my laptop 20 ms. Which is superfast, given the fact that it is generated only once when the plugin is loaded and every time the user changes one of the relevant settings, requiring producing a new parser. So very seldom.

[alberti42]

The other day I was in a hurry and had to leave quickly. I checked the messages generated in the test again. It seems to me that all tests are passed. There is some red warning because of some part of code that are never reached. This concerns code when using try / catch construct. The catch part is only reached and executed if an error is produced. One would need to include in the test modules some tests generating on purpose the error to execute the code in the catch part. Or alternatively, we can remove the try+catch construct (which is not good style either).

So I think it is not so important, and it is very time consuming to write test modules that verify all try and catch constructs. Did you do so before? Or simply avoided using try/catch constructs?

I show you a screenshot of what the result of the tests is:

Will you handle the integration of the pull request into the main branch? Is there anything else you want me to do?

[alberti42]

Ok. I fetched the upstream changes, and made a single commit with all changes for the new parser.

[alberti42]

Ok. I understood that I need to do some more changes on the test routines. Also my latest commit does not include some changes I did before which I need to reintroduce. I need some more time to finish it. It should not last long.

[alberti42]

I made one more commit. I believe everything works. However, many tests are still not passing because the test routines need to be updated to integrate with the new syntax of the parser.

I am afraid I don't have anymore time to invest in this project.

Regarding the test modules, it is often very difficult for me to understand the reasons of certain rules that seem to be inserted ad hoc. For example, I had to debug a test in tests/unit/FlashcardReviewSequencer.test.ts saying:

test("Question starts line after tag (single space after tag before newline); New card", async () => {
                let originalQuestionStr: string = `#flashcards${space}
Q2
?
A2`;

                let fileText: string = `
${originalQuestionStr}

#flashcards Q1::A1

#flashcards Q3::A3`;

                let updatedQuestionText: string = `Multiline question
Question starting immediately after tag
?

It was not clear to me at all why if I give:

#flashcards<space here>
Q2
?
A2

#flashcards Q1::A1

#flashcards Q3::A3

then it expects something without the space after #flashcards. Why to make such a big deal for the space? And why to remove it? Why does it bother? It is hard to understand these rules and generalize them without creating ad hoc solutions. For the case presented above, I created an ad hoc rule recognizing the tags in a multiline card. But it seems to me not the right approach. For sure I am missing something else.

Also, ideally you would like to parse Obsidian tags with the proper parser. I already made a rule in the grammar.

And the frontmatter could be parsed as well with peggy rules without creating any problem. It does not need to be treated separately.

I need your support. We should reduce the test cases and streamline the code. I myself cannot invest any more time in this plugin. I am sorry.

In the worst case, I will create a new plugin for spaced repetitions from scratch with a shorter and cleaner code.

[4Source]

I try to help you with writing and fixing some of the test cases I hope I can push to this pr, if not I will create one on your fork

[4Source]

(Note: I've never had to restore a branch to a previous state. Hopefully that can be done safely.)

@ronzulu Not sure this helps you but you can revert a commit. Alternatively you can reset branch without losing the changes not sure how good this works for already pushed commits
I would recommend first trying to use reset keeps the history cleaner
git reset HEAD~<number of commits to go back> Keeps the changes as staged
git revert <commit> creates new commit which undo all back to the commit

[ronzulu]

Hi @alberti42

I think I've found a bug whilst writing the user notes.

Here is a multiline card with a table in the answer...

Table with preceding blank line and+++.md

This displays correctly during review...

But after clicking on hard, the scheduling info isn't added to the file, and this error is shown in the error log

obsidian.md-1724468486804.log

Cheers
Ronny

[alberti42]

Hi Ronny,

Good that you found the bug. Can you provide the text of your flash card and how you configured the delimiters? In other words, are you using the default settings? Or you chose some special character/symbol?

I will have a look at it.

PS: I can reproduce the bug. No need more examples from your side.

[alberti42]

To my understanding it has nothing to do with the tables.

Testing card
?

Line 1
Line 2

---

The problem seems to be when you have multicards with empty lines and there is an empty line after ?. I will check the details.

[alberti42]

Hi Ronny,
I found the problem. It was my fault. I used .trim() instrad of .trimEnd() in the parser. I fixed the problem and added one more test case.

All tests now pass (commit: f700346).

[ronzulu]

Hi @alberti42 I've completed changes to the user guide at:
https://github.com/ronzulu/obsidian-spaced-repetition/tree/rz-alberti42-v2

Feel free to merge and make any changes.

The end is in sight!

[alberti42]

Wonderful. I accepted all your changes. All tests are green!!

[st3v3nmw]

Thanks @alberti42, @ronzulu & @4Source for the work put in!

[ronzulu]

Thanks @alberti42, @ronzulu & @4Source for the work put in!

I think if this was IRL we would be going to the pub for a drink to celebrate :-)

[ragrawal]

So do I just need to reinstall the plugin to get the latest version of the spaced repetition plugin

[alberti42]

I think if this was IRL we would be going to the pub for a drink to celebrate :-)

🎉🎉🎉 Absolutely 😀

[alberti42]

So do I just need to reinstall the plugin to get the latest version of the spaced repetition plugin

I don't think you need to uninstall it. However, you need to wait for the upstream changes to be part of the new release. I am not sure when this will happen, but I celebrate that the changes are now part of the upstream and the release will happen at some point.

[ragrawal]

wondering when will this be available. Seems like the updated the plugin was never released. The github page shows the last release to be 1.12.5 which was about a month ago

[ronzulu]

This is now part of the latest beta release:
#1011

[ronzulu]

wondering when will this be available. Seems like the updated the plugin was never released. The github page shows the last release to be 1.12.5 which was about a month ago

Hi @ragrawal, wondering if you've had a chance to try this out yet. #1011

[ragrawal]

I started using it. Thanks. It’s working great. Didn’t notice any issue .