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

Potential bugs with header block parsing #329

Open
julianjuko opened this issue Sep 6, 2023 · 10 comments
Open

Potential bugs with header block parsing #329

julianjuko opened this issue Sep 6, 2023 · 10 comments
Labels
bug Something isn't working enhancement New feature or request

Comments

@julianjuko
Copy link

julianjuko commented Sep 6, 2023

The code related to parsing blocks under headers has the following line:

const level = line.split('#').length - 1;

This splits the line at each # to determine the header's level, assuming that all # symbols represent heading levels. Any # symbols that are not at the start of the line would incorrectly alter the calculated header level.

Presumably, this will cause headers such as

### Thoughts on the #metoo movement

to be level: 4 when it should be level: 3.

As I have not tested this, I will close this issue if I am mistaken - I mainly wanted to bring it to your attention. Thanks for the time spent working on this plugin.

@julianjuko julianjuko changed the title Potential bug with header block parsing Potential bugs with header block parsing Sep 6, 2023
@julianjuko
Copy link
Author

julianjuko commented Sep 6, 2023

Also, as the current format for the block_headings string appears to be akin to #Top Heading#Heading#Subheading#, if a user were to put "Heading" in their header_exclusions setting field, this line

if (block_headings.indexOf(this.header_exclusions[k]) > -1) {

would not only exclude headers that are titled Heading, but also header strings that include this header exclusion, such as Top Heading in the above case. This probably isn't the behaviour that users of this feature expect (for example, me, who is thrilled that you added it, btw!)

@julianjuko
Copy link
Author

julianjuko commented Sep 6, 2023

Finally, a description of how the header_exclusions setting works would help a lot. I originally started looking at the code for this to try and figure out if the format for header_exclusions was something that allows you to specify the header level, such as
"## Example code,# Tasks"
or whether it does a basic text match without header levels, such as
"Example code,Tasks"

From what I've gathered, it appears to be the latter currently. I would love for the former to be implemented, however.

@brianpetro
Copy link
Owner

@julianjuko that's all very helpful feedback!

Normally I'd address the things one by one, but each of your concerns/suggestions look relatively complete and accurate.

I will keep these in mind as I move forward with updating Smart Connections.

Thanks for taking the time to put these notes together and helping make Smart Connections the best it can be 🤓

🌴 Brian

@brianpetro brianpetro added bug Something isn't working enhancement New feature or request labels Sep 6, 2023
@RealRavens
Copy link

as I move forward with updating Smart Connections.

👀

We'll believe it when we see it... 😆😜

Keep at it Brian 👍

@brianpetro
Copy link
Owner

@RealRavens 😂

Big things happen all at once 😉

@1ncontinentia
Copy link

Hi! Just adding in here—I can't get header exclusion settings to work regardless of the format I use lately. For each note I have a header with "# Metadata" at the top, most of the content of which overlaps with other notes, and so all Smart Connections does really is link these Metadata sections to each other. I've tried using "Metadata" and "# Metadata" in the header exclusion settings without it working. Any guidance would be appreciated! And thanks for the epic work!! This is such a game-changingly helpful plugin, really appreciate all the hard work @brianpetro.

@brianpetro
Copy link
Owner

@1ncontinentia thanks for the kind words about Smart Connections 😊

Are you using note-level embeddings only? Or block-level emeddings, too?

The header exclusion will only impact block-level embeddings. Preventing excluded headings from being used in note-level embeddings would be a good feature, though.

There might be some other situations where the excluded heading is used since the v2.0 update didn't implement any new logic for this feature, though a lot of other things changed.

I also plan to implement exclusions for meta variables themselves. Which it seems like is the primary reason you want to exclude headings is to address this metadata issue.

🌴

@1ncontinentia
Copy link

Hi @brianpetro, cheers for the reply! The issue is with block-level embeddings actually, the note-level embeddings work fine. A few versions back it wasn't an issue—I guess pre-2.0. At that time, the Smart Connections tab just showed non-excluded blocks as intended. Currently I have to disable block embeddings or the 10 most relevant blocks for most of my notes end up being either the "Metadata" block or the "Dataview" block from lots of different notes. (For context, I have dataview tables in many of my notes which have block header exclusions too).

@brianpetro
Copy link
Owner

@1ncontinentia Thanks for the follow-up, that's good to know. I'll have to look into this further.

@1ncontinentia
Copy link

1ncontinentia commented Mar 25, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants