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

[Bug]: Detecting inside codeblocks since last update #1072

Closed
dmantisk opened this issue Sep 16, 2024 · 17 comments · Fixed by #1081
Closed

[Bug]: Detecting inside codeblocks since last update #1072

dmantisk opened this issue Sep 16, 2024 · 17 comments · Fixed by #1081
Labels
bug Something isn't working

Comments

@dmantisk
Copy link

Describe the bug

After the last update the both the inline and multi line flashcard indicators are being detected.
Using default or custom indicators doesn't seem to matter.

Is this intended behavior?

If so I'll just switch the indicators to something else.

To Reproduce

  1. Create a code block using triple back ticks
  2. Use inline or multiline indicator

Expected behavior

The indicators not being detected inside codeblocks

Screenshots

No response

OS of your device

Win10

Obsidian version

1.6.7

Plugin version

1.12.6

Installer version

1.6.5

Additional context

No response

Config file

No response

@dmantisk dmantisk added the bug Something isn't working label Sep 16, 2024
@ronzulu
Copy link
Collaborator

ronzulu commented Sep 17, 2024

Hi @dmantisk thanks for reporting that.

There was no intention (AFAIK) to modify this behavior. Likely, there was no pre-existing test case for this condition when Added new option allowing for multiline cards with empty lines #1012 was implemented, and this change wasn't noticed.

I know you have included info in the "To Reproduce" section, but it's easiest if you simply include a sample file. Best if you can attach a markdown file (rather than copy and paste).

@alberti42 what are your thoughts?

Cheers
Ronny

@dmantisk
Copy link
Author

Untitled.md

This file uses #fc as the tag for identifying the note as a flashcard containing note
I've used the default :: as the inline flashcard indicator.

The contents of the file are a normal inline flashcard, one inside fenced codeblock, and one inside inline codeblock.

I hope that helps

@alberti42
Copy link
Contributor

Yes, I could reproduce the bugs, found the problem and found (I believe) the solution.

In general, if you encounter similar problems in the future, the typical pattern for debugging this is to go in debug mode:

Bildschirmfoto 2024-09-17 um 07 04 03

You play some flashcards and then check on the console what is produced:

Bildschirmfoto 2024-09-17 um 07 03 52

In particular, you are interested in copying the content of grammar with right click into https://peggyjs.org/online.html

There you can test your flash card and understand why and what is not identified.

alberti42 added a commit to alberti42/obsidian-spaced-repetition that referenced this issue Sep 17, 2024
@alberti42
Copy link
Contributor

I posted the solution with this commit. I checked it passes all tests.

It would be good to add some more test cases. This will avoid getting back in this situation in the future if we modify the grammar.

@ronzulu
Copy link
Collaborator

ronzulu commented Sep 20, 2024

Thanks, I'm happy to write some additional test cases.

st3v3nmw pushed a commit that referenced this issue Sep 20, 2024
…menu options (#1066)

* Added options in the settings to hide icon and status bar.

* - Enabled the possibility to disable file menu options without restarting Obsidian or reloading the plugin.
- Changed the phrasing for the user from `Disable` to `Enable` to make it easier to understand and aligned with the other options.

* Adjusted the translation in the different languages.

* removing unnecessary logs

* Fixed bug #1072
st3v3nmw pushed a commit that referenced this issue Sep 21, 2024
…menu options (#1066)

* Added options in the settings to hide icon and status bar.

* - Enabled the possibility to disable file menu options without restarting Obsidian or reloading the plugin.
- Changed the phrasing for the user from `Disable` to `Enable` to make it easier to understand and aligned with the other options.

* Adjusted the translation in the different languages.

* removing unnecessary logs

* Fixed bug #1072
st3v3nmw added a commit that referenced this issue Sep 21, 2024
This PR:
1. Adds test cases for parsing {{curly}} clozes
2. Fixes #1072
    - Ignore 'cards' in ```block`` & `inline` codeblocks
    - Adds test cases for both
3. Fixes #1077
    - When all the cloze options aren't being used, `cloze_rules` in
     `parser.ts` becomes empty which leads to the empty grammar rule `cloze_text`
     which prevents the grammar from being generated. This change adds a
     tombstone \0 to address this.
    - Adds test cases for this
st3v3nmw added a commit that referenced this issue Sep 21, 2024
This PR:
1. Adds test cases for parsing {{curly}} clozes
2. Fixes #1072
    - Ignore 'cards' in ```block`` & `inline` codeblocks
    - Adds test cases for both
3. Fixes #1077
    - When all the cloze options aren't being used, `cloze_rules` in
     `parser.ts` becomes empty which leads to the empty grammar rule `cloze_text`
     which prevents the grammar from being generated. This change adds a
     tombstone \0 to address this.
    - Adds test cases for this
@st3v3nmw
Copy link
Owner

@alberti42 @ronzulu

I've added some extra test cases in #1081 & also updated the parser to prevent detection of cards in inline code. I'm no expert with peggy grammars so it'd be great if someone reviewed it before merging. Thanks.

st3v3nmw added a commit that referenced this issue Sep 21, 2024
This PR:
1. Adds test cases for parsing {{curly}} clozes
2. Fixes #1072
    - Ignore 'cards' in ```block`` & `inline` codeblocks
    - Adds test cases for both
3. Fixes #1077
    - When all the cloze options aren't being used, `cloze_rules` in
     `parser.ts` becomes empty which leads to the empty grammar rule `cloze_text`
     which prevents the grammar from being generated. This change adds a
     tombstone \0 to address this.
    - Adds test cases for this
@alberti42
Copy link
Contributor

@st3v3nmw
Thanks! I am checking it.

@alberti42
Copy link
Contributor

I don't understand your changes. Was there any reason to modify my suggestion? Did you see some cases where it failed?

Why did you need the tombstone case? Its usage is not well documented in the code and alone from the name, I cannot understand what the purpose is. I presume it is a patch when no cloze rule is provided.

To be more specific, I am confused why you changed:

inline
= $(left:(!inline_mark (inline_code / non_newline))+ inline_mark right:text_till_newline (newline annotation)?) {
  return createParsedQuestionInfo(CardType.SingleLineBasic,text(),location().start.line-1,location().end.line-1);
}

The syntax (!inline_mark (inline_code / non_newline))+ means that it will advance char by char and check that the next string does not contain inline_mark. It swallows any char, as I just said, but also (and this was the change I made recently) it swallows an inline_code piece of text.

I think this is exactly what you want to happen. If you want to detect something like:

my inline question containing `some inline code` in it::and this is answer possibly containing `inline` code.

you advance character by character, and then if you find an inline code before :: you swallow all of it and then advance through the remaining text until ::.

Note that the order (inline_code / non_newline) matters. The opposite (non_newline / inline_code) does not work because non_newline is always matched when inline_code is matched too.

So, I would like to hear your thoughts why you did the changes on my code, before I go through detailed debugging of your grammar.

@alberti42
Copy link
Contributor

Ok, I think I now understand the tombstone, but I think I can propose a better/more elegant solution. Let me check this.

Instead, let me know your thoughts on the rest that I posted above.

@alberti42
Copy link
Contributor

I also understood that my changes before did not fix the case of inline code in clozes. They only handled the multiline cards. Thanks spotting it the remaining problem. However, I am not sure your solution is the best. I will make a suggestion.

@st3v3nmw
Copy link
Owner

@alberti42, thanks!

Was there any reason to modify my suggestion? Did you see some cases where it failed?

Yes, the failing cases were when it detected cloze cards inside inline code blocks when the respective cloze options (convertHighlightsToClozes, convertBoldTextToClozes, & convertCurlyBracketsToClozes) are true. For instance:

if a == b && c == d {}
z = a ** b + 5 ** 7
foo = {{'a': 2}}

Per your fix, it doesn't pick up cards inside the ``` codeblocks.

Why did you need the tombstone case? Its usage is not well documented in the code and alone from the name, I cannot understand what the purpose is. I presume it is a patch when no cloze rule is provided.

Yes, that was a hack to not detect clozes when the cloze options are all off.

The errors being thrown looked like this because cloze_text = :

plugin:obsidian-spaced-repetition:9740 Unexpected error: SyntaxError: Expected ":", comment, end of line, or whitespace but "=" found. (at plugin:obsidian-spaced-repetition:5023:23)
at new peg$SyntaxError (plugin:obsidian-spaced-repetition:5023:23)
at eval (plugin:obsidian-spaced-repetition:5553:18)
at Object.peg$parse [as parse] (plugin:obsidian-spaced-repetition:5554:10)
at eval (plugin:obsidian-spaced-repetition:6259:27)
at Array.map ()
at generate (plugin:obsidian-spaced-repetition:6257:32)
at generateParser (plugin:obsidian-spaced-repetition:9450:43)
at parseEx (plugin:obsidian-spaced-repetition:9732:21)
at NoteQuestionParser.parseQuestions (plugin:obsidian-spaced-repetition:11753:20)
at NoteQuestionParser.doCreateQuestionList (plugin:obsidian-spaced-repetition:11719:41)
at NoteQuestionParser.createQuestionList (plugin:obsidian-spaced-repetition:11695:32)
at async NoteFileLoader.load (plugin:obsidian-spaced-repetition:11923:26)
at async OsrAppCore.loadNote (plugin:obsidian-spaced-repetition:12900:18)
at async OsrAppCore.processFile (plugin:obsidian-spaced-repetition:12825:14)
at async OsrAppCore.loadVault (plugin:obsidian-spaced-repetition:12934:9)
at async _SRPlugin.sync (plugin:obsidian-spaced-repetition:26251:5)
at async eval (plugin:obsidian-spaced-repetition:26052:11)

Ok, I think I now understand the tombstone, but I think I can propose a better/more elegant solution. Let me check this.

Of course, a more elegant solution would be better 😄. In the meantime, I'll add a comment in the code.

To be more specific, I am confused why you changed:

inline
= $(left:(!inline_mark (inline_code / non_newline))+ inline_mark right:text_till_newline (newline annotation)?) {
  return createParsedQuestionInfo(CardType.SingleLineBasic,text(),location().start.line-1,location().end.line-1);
}

I was trying to prevent it from picking up "cards" inside inline code. Like the examples above. I'll revert the change & figure out a better way to handle that.

I think this is exactly what you want to happen. If you want to detect something like:

my inline question containing `some inline code` in it::and this is answer possibly containing `inline` code.

I hadn't considered this case. I've added it to the test cases which is now failing due to my change 😭.

st3v3nmw added a commit that referenced this issue Sep 21, 2024
This PR:
1. Adds test cases for parsing {{curly}} clozes
2. Fixes #1072
    - Ignore 'cards' in ```block`` & `inline` codeblocks
    - Adds test cases for both
3. Fixes #1077
    - When all the cloze options aren't being used, `cloze_rules` in
     `parser.ts` becomes empty which leads to the empty grammar rule `cloze_text`
     which prevents the grammar from being generated. This change adds a
     tombstone \0 to address this.
    - Adds test cases for this
alberti42 added a commit to alberti42/obsidian-spaced-repetition that referenced this issue Sep 21, 2024
… not properly hanlded by clozes

- Fixed a new issue related to the case when the user provides empty markers for the inline and multiline cards. With the new version, the rule is disabled when the marker is empty.
@alberti42
Copy link
Contributor

I made a new version that hopefully fixes the problem you found and also fix a new problem, see the comment to the commit.

alberti42@411aa64

Thank you for the explanation! When you posted it, I had already code the version in the commit above. So, I think it is worth you test it.

I could not run anymore the tests. When I run:

npm run test

it says that you now moved to pnpm. I installed it, but never used it. What should I type on the command line to run the tests?

npm run build

still works for me.

@alberti42
Copy link
Contributor

I did not understand your comment:

Per your fix, it doesn't pick up cards inside the ``` codeblocks.

In my view, if flashcard markers appear inside a codeblock, these should be ignored. However, I think we should definitly allow the user to have codeblocks in both the question and answer, but the content inside should not have any influence on what cards are shown, it is just a codeblock.

@st3v3nmw
Copy link
Owner

see the comment to the commit.

  • Fixed a new issue related to the case when the user provides empty markers for the inline and multiline cards. With the new version, the rule is disabled when the marker is empty.

I've added test cases for this.

it says that you now moved to pnpm. I installed it, but never used it. What should I type on the command line to run the tests?

# install dependencies
pnpm i

# run tests
pnpm run tests

# build
pnpm run build

In my view, if flashcard markers appear inside a codeblock, these should be ignored. However, I think we should definitly allow the user to have codeblocks in both the question and answer, but the content inside should not have any influence on what cards are shown, it is just a codeblock.

Yes, this is what I meant. We're ignoring markers inside the codeblocks but allowing users to have codeblocks in their Qns & Answers.

@st3v3nmw
Copy link
Owner

@alberti42, I've ran the tests with the new test cases and they all pass (#1081). I haven't modified the grammar so I think it's okay to make a release now. Thank you for the fixes & quick response.

@st3v3nmw
Copy link
Owner

@dmantisk, please try the new release (v1.12.7) and let us know if it fixes the issue for you.

@dmantisk
Copy link
Author

I can confirm that the issue is fixed.

Flashcard indicators inside both inline and fenced code blocks are no linger being picked up.
Thank you!

Newdea pushed a commit to open-spaced-repetition/obsidian-spaced-repetition-recall that referenced this issue Sep 25, 2024
…menu options (st3v3nmw#1066)

* Added options in the settings to hide icon and status bar.

* - Enabled the possibility to disable file menu options without restarting Obsidian or reloading the plugin.
- Changed the phrasing for the user from `Disable` to `Enable` to make it easier to understand and aligned with the other options.

* Adjusted the translation in the different languages.

* removing unnecessary logs

* Fixed bug st3v3nmw#1072
Newdea pushed a commit to open-spaced-repetition/obsidian-spaced-repetition-recall that referenced this issue Sep 25, 2024
* fix: parsing code blocks & custom separators

This PR:
1. Adds test cases for parsing {{curly}} clozes
2. Fixes st3v3nmw#1072
    - Ignore 'cards' in ```block`` & `inline` codeblocks
    - Adds test cases for both
3. Fixes st3v3nmw#1077
    - When all the cloze options aren't being used, `cloze_rules` in
     `parser.ts` becomes empty which leads to the empty grammar rule `cloze_text`
     which prevents the grammar from being generated. This change adds a
     tombstone \0 to address this.
    - Adds test cases for this

* Fixed inline code in clozes

* - Fixed issue raised at st3v3nmw#1072 (comment) where inline code was not properly hanlded by clozes
- Fixed a new issue related to the case when the user provides empty markers for the inline and multiline cards. With the new version, the rule is disabled when the marker is empty.

* add parser test case

* add more parser test cases

---------

Co-authored-by: Andrea Alberti <a.alberti82@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants