Feat #879: Different embedding styles prep #1273

badsketch · 2023-08-18T04:34:48Z

Addresses #879
This PR does not change functionality. It's mainly implementation changes under the hood in preparation for when we do add other embedding styles.

I tried to keep in mind that we want to make this extensible for future variations of style and content scoping, so I tried to use a strategy design pattern where we have extractors and formatters. Hoping it's not overkill and not too hard to read. 😅OOP isn't my strong suit, but I had fun! Hoping for feedback.

Is this what you had in mind? Could you also clarify what you think would be the correct rolloutstrategy? I noticed if I add a deprecationMessage to preview.embedNoteInContainer then it disappears from the settings UI unless you explicitly have it set. At the moment, the new preview.embedStyle doesn't do anything. Wondering at what point should I hot swap the two.
If we're going to maintain both at the same time, (before preview.embedNoteInContainer gets removed), which setting should take precedence?

packages/foam-vscode/src/features/preview/wikilink-embed.ts

badsketch · 2023-08-18T04:39:19Z

packages/foam-vscode/src/features/preview/wikilink-embed.ts

@@ -47,25 +47,20 @@ export const markdownItWikilinkEmbed = (
        let content = `Embed for [[${wikilink}]]`;
        switch (includedNote.type) {
          case 'note': {
-            let noteText = readFileSync(includedNote.uri.toFsPath()).toString();
-            const section = Resource.findSection(
+            const noteStyle = getFoamVsCodeConfig(


In the next phase, I'd update the regex to capture the new syntax. Then here we'd have logic to determine noteStyle and noteContent. For now, noteStyle pulls from the existing config setting, and there is no noteContent, it just defaults to "full".

That's correct, although from the configuration we should be getting both the style and the content, is that planned for this PR?

So I'm a little fuzzy on release strategy. The new preview.embedStyle isn't used anywhere at the moment, but I can change it so that this wikilink-embed reads it and it overrides preview.embedNoteInContainer if both are set. I can also add a deprecation flag to preview.embedNoteInContainer so it no longer appears in settings UI. In the next PR, I can remove it altogether package.json as well as this file. wdyt?

The release strategy you outlined is what I had in mind and I think works pretty well.

I am not aware of a built-in way in VS Code to do that, did you come across something or had something in mind?

Do you mean for the deprecation flag? https://code.visualstudio.com/api/references/contribution-points#contributes.configuration:~:text=deprecationMessage%20/%20markdownDeprecationMessage

Yes, that's great, I was already thinking of a small utility to deal with deprecation, but that won't be necessary 👍

riccardoferretti

Thanks for taking a stab at this!
For what regards the design, in Foam we tend to be more on the functional side than OOP (with blurred lines, especially because VS Code API is mostly OO).
I like the separation of the extraction from the rendering.
For me an interface with a single method is a strong candidate for being replaced by a function type (e.g. see FoamFeature interface, it used to be an object, in the end I decided that a simple fn was enough for all the use I needed for it, and made the code more readable).

I left some comments, let me know your thoughts

riccardoferretti · 2023-08-18T14:52:18Z

packages/foam-vscode/src/features/preview/wikilink-embed.ts

+
+class InlineFormatter implements EmbedNoteFormatter {
+  format(content: string) {
+    return content;


I think you still wanna render here right? this.md.render(content)

I think the switch statement for note, attachment, and image all return content and outside the switch statement content gets rendered here https://github.com/foambubble/foam/pull/1273/files#diff-42b32f07212d469a3715b0a74b0f67ff192e592d0724fcfce358130debe8dcfaL84

It does feels weird for InlineFormatter to be doing nothing, but the Extractor kind of already does everything and I couldn't find something in

extract(note: Resource) { let noteText = readFileSync(note.uri.toFsPath()).toString(); const section = Resource.findSection(note, note.uri.fragment); if (isSome(section)) { const rows = noteText.split('\n'); noteText = rows .slice(section.range.start.line, section.range.end.line) .join('\n'); } noteText = withLinksRelativeToWorkspaceRoot( noteText, this.parser, this.workspace ); return noteText; // already "formatted" } }

that could belong in formatter.

CardFormatter renders the md, so I feel the behavior should be consistent: do we expect the formatter to return an html string or a md string?

So the issue is that card style needs an additionalmd.render(content) step.
Existing code returns an md string:

content = getFoamVsCodeConfig(CONFIG_EMBED_NOTE_IN_CONTAINER) ? `<div class="embed-container-note">${md.render(noteText)}</div>` : noteText;

So we can have both return an html string:

function cardFormatter(content: string, md: markdownit): string { return md.render( `<div class="embed-container-note">${md.render(content)}</div>` ); } function inlineFormatter(content: string, md: markdownit): string { return md.render(content); }

or we can have both return an md string

function cardFormatter(content: string, md: markdownit): string { return `<div class="embed-container-note">${md.render(content)}</div>` } function inlineFormatter(content: string, md: markdownit): string { return content; }

Oh I see what you mean, I had forgotten about the double rendering.
I don't have a strong preference between the two options, so long as we are consistent, happy to go with what you prefer.

cool, I decided to go with producing an html and having the rest of the cases like embedding images or attachments also produce the html as well

packages/foam-vscode/src/features/preview/wikilink-embed.ts

riccardoferretti · 2023-08-18T15:23:37Z

packages/foam-vscode/src/features/preview/wikilink-embed.ts

@@ -47,25 +47,20 @@ export const markdownItWikilinkEmbed = (
        let content = `Embed for [[${wikilink}]]`;
        switch (includedNote.type) {
          case 'note': {
-            let noteText = readFileSync(includedNote.uri.toFsPath()).toString();
-            const section = Resource.findSection(
+            const noteStyle = getFoamVsCodeConfig(


That's correct, although from the configuration we should be getting both the style and the content, is that planned for this PR?

riccardoferretti · 2023-08-18T15:25:46Z

packages/foam-vscode/src/features/preview/wikilink-embed.ts

+            )
+              ? 'card'
+              : 'inline';
+            const noteEmbedder = new NoteEmbedder(


I wonder if NoteEmbedder helps encapsulate a behavior, and compare that with the additional "complexity" of the indirection - what do you think?

A little confused by "encapsulate a behavior". Is that a type of implementation pattern?

no no, what I mean is basically that currently in NoteEmbedder lives the code that was in this function (that's what I mean by "encapsulating": we don't see the actual code and rely on this object to "do the work").
This indirection removes complexity by removing some code here (and putting it in NoteEmbedder) but also adds complexity because understanding the code requires an extra step (that is, check the implementation of NoteEmbedder).
My original comment was whether what's gained offsets what's lost, I wonder if we could in fact remove the NoteEmbedder and simply have the code here, wdyt?

Gotcha, I see what you mean. That's a good point. For e05c1c2 I changed the interfaces to function types and replaced all the classes with functions. Is this what you had in mind?

I feel like the class-based approach would be preferable after we add more variations, so maybe we can go with the functional approach and reconsider it as an option in the future? I say that because with the functional approach, it seems dependencies get added for all formatters and extractors. Like if extractor A requires parser: ResourceParser, but extractor B doesn't, the type still needs to include it, and extractor B has to have it in its signature:

export type EmbedNoteExtractor = ( note: Resource, parser: ResourceParser, ) => string; function extractorA(note: Resource, parser: resourceParser) {} function extractorB(note: Resource, parser: resourceParser) {} // doesn't need the parser, but generateNoteEmbedding() expects a type EmbedNoteExtractor

How about moving the two lines of generateNoteEmbedding in the caller and remove the fn altogether?

Your comment about the dependencies is correct. And in some of my functions this situation happens. For example in the feature activate function I used as example before, sometimes the Foam object is not required, but I pass it anyways.
Depending on the number of dependencies and how related they are to the method, I am ok with the compromise.

In this specific case of the extractor and the formatter, so far the parameters being passed IMO read well with the function signature, so I don't see yellow/red flags.
This is something that can be reassesed in the future shall the need arise.

As an aside, the functional way of mimicking the constructor would be using a factory, e.g.

function createExtractor(extraDep: any) : (note: Resource, parser: resourceParser) => string { return (note: Resource, parser: resourceParser) => { // do something with all extraDep, note and parser } } extractorC = createExtractor(extra) extractorC(note, parser)

Great suggestion, done! Also really appreciate the snippet

badsketch · 2023-08-18T22:12:50Z

For me an interface with a single method is a strong candidate for being replaced by a function type (e.g. see FoamFeature interface, it used to be an object, in the end I decided that a simple fn was enough for all the use I needed for it, and made the code more readable).

Great idea! I think it'll make it a lot more succinct

badsketch · 2023-08-19T04:15:02Z

For me an interface with a single method is a strong candidate for being replaced by a function type (e.g. see FoamFeature interface, it used to be an object, in the end I decided that a simple fn was enough for all the use I needed for it, and made the code more readable).

Great idea! I think it'll make it a lot more succinct

Hmm, gave this shot, but I realized that with a function type I don't have a way to enforce classes to implement that function. And NoteEmbedder expects the interfaces to be defined so that it can use polymorphism to extract() and format().

class NoteEmbedder {
  extractor: EmbedNoteExtractor; // can't be a function type
  formatter: EmbedNoteFormatter; // can't be a function type
.
.
 generateEmbedding () {
  content = extractor.extract(stuff)
  return formatter.format(content)
}

Could I be missing something?

riccardoferretti · 2023-08-20T12:07:10Z

I think I replied to all your questions, let me know if the part around the embedder is still unclear though!

packages/foam-vscode/package.json

badsketch · 2023-08-20T22:48:04Z

Appreciate your patience in working through this with me! I think the code/design is in a good spot based on what we discussed 👍

Lastly:
I decided to change preview.embedStyle to preview.embedNoteType since in the code I use "style" to refer to the format type (inline, card), and the naming was starting to get confusing to me if the setting has the word "style" in it. I thought it might be a clearer separation of identity for our variables if scope = full/content, style = inline/card, and embedNoteType as the combination of the two. Thoughts?

Is it possible for you to retrigger the CI /Build and Test? It's failing on wikilink-embed.spec.ts, but it succeeds on my machine. Also, I'm noticing refactor.spec.ts fail on my machine, but succeed in CI. Have you noticed this before along with a lot of listener failed errors? Or might I have messed something up 😓

riccardoferretti

Yes it looks really good!
I left a couple of minor comments, but we are there.

packages/foam-vscode/src/features/preview/wikilink-embed.ts

riccardoferretti · 2023-08-21T09:23:51Z

packages/foam-vscode/src/features/preview/wikilink-embed.ts

+            let formatter: EmbedNoteFormatter = cardFormatter;
+            switch (noteStyle) {
+              case 'card':
+                formatter = cardFormatter;
+                break;
+              case 'inline':
+                formatter = inlineFormatter;
+                break;


a more condensed way to achieve this (we use it in other places in the Foam codebase):

const formatter = noteStyle === 'card' ? cardFormatter : noteStyle === ' inline' ? inlineFormatter : cardFormatter // your default

It's purely a matter of style, for me the advantage is that it allows to use const (a bummer that js/ts don't have a functional switch statement), it's a bit more clear that the conditions are just about initiating the variable, and that the conditions can a bit more flexible. I think it takes a sec getting used to, but it flows once the eye is trained :)

For consistency with the codebase I would lean towards using the pattern for both extractor and formatter.

see link-completion.ts:188 and generate-link-references.ts:68

I'm all for consistency! I can't help but unravel ternary operators in my head, which make chained ones seem complex, but you're right, they start looking just like an if/else block in time. Updated!

riccardoferretti · 2023-08-21T09:30:07Z

I re-triggered the tests, it looks like something is getting escaped in the html, so I reckon it will fail again.

(re the refactor tests, yes sometimes they fail for me too locally, haven't figured out how to remove those spurious fails)

…f html content

…nstead of html content" This reverts commit 2598689.

…k if there's a caching issue

…eck if there's a caching issue

badsketch · 2023-08-22T07:01:37Z

You might be able to tell from the commit history I've somewhat lost it 😓. I've just been getting the most bewildering results in terms of reproducibility, which makes it really frustrating to debug, and it requires I push to really see if my changes are effective. All e2e tests pass for Windows and Mac OS X for me, but as you metnioned, CI keeps failing due to the escaped html tags.

I'm starting to suspect it's the async nature of wikilink-embed.spec.ts and they way it sets up the dependencies(?), but knowing my luck it could just be another red herring. Might take a bit longer as I try to fix this. If it's alright with you, I might create a dummy branch with cherry-picked commits then create a dummy PR to see if I can reproduce it with Github CI. Let me know if I missed something obvious!

riccardoferretti · 2023-08-22T09:36:12Z

I took a look at the tests and it seems like they are using the card formatter instead of the inline.
And I wonder if the problem is in retrieveNoteConfig, probably we shouldn't give priority to the old setting CONFIG_EMBED_NOTE_IN_CONTAINER.

(2m later - I did a quick test locally by commenting the overwrite and the tests passed)

…config

…ntainer config" This reverts commit 4f773f9.

…in the tests

…ner in the tests

This reverts commit f86dc7e.

badsketch · 2023-08-23T03:55:23Z

Yeah! Based on what you said, I think I got it figured out. First mistake was setting the wrong config name 4190c43, and secondly, I needed to initialize CONFIG_EMBED_NOTE_IN_CONTAINER, by wrapping the tests in an additional withModifiedFoamConfiguration() otherwise all had the setting set to true, and the flag has higher precedence in retrieveNoteConfig()

Should finally be in a good review state. Again, thanks for the troubleshooting help! Looking forward to finally doing the implementation 😅

…ive state

riccardoferretti · 2023-08-23T14:51:02Z

packages/foam-vscode/src/features/preview/wikilink-embed.ts

+
+  // **DEPRECATED** setting to be removed
+  // for now it overrides the above to preserve user settings if they have it set
+  if (getFoamVsCodeConfig<boolean>(CONFIG_EMBED_NOTE_IN_CONTAINER, false)) {


From this I assume that if someone has both settings, the old setting will overwrite the new one?
I think that the new setting should always have the precedence, and we should only read the previous one if the new setting has not been used, wdyt?

riccardoferretti

On second thought, I don't think that comment is very important, as we are planning to sunset the old config pretty quickly, so all looks good to me!

Refactor note embedding to be extensible

4e2f07a

badsketch commented Aug 18, 2023

View reviewed changes

packages/foam-vscode/src/features/preview/wikilink-embed.ts Show resolved Hide resolved

badsketch commented Aug 18, 2023

View reviewed changes

riccardoferretti reviewed Aug 18, 2023

View reviewed changes

Refactor pattern to use more functional approach

e05c1c2

badsketch added 3 commits August 20, 2023 15:03

Simplify extractor formatter invocation

33e4438

Prepare new setting to replace preview.embedNoteInContainer

e3fe400

Fix wikilink-embed e2e tests

3b1c903

badsketch commented Aug 20, 2023

View reviewed changes

packages/foam-vscode/package.json Show resolved Hide resolved

badsketch marked this pull request as ready for review August 20, 2023 22:48

riccardoferretti reviewed Aug 21, 2023

View reviewed changes

badsketch added 8 commits August 21, 2023 22:56

Improve readability

7d97e78

Try to fix wikilink-embed e2e tests by returning md content instead o…

2598689

…f html content

Revert "Try to fix wikilink-embed e2e tests by returning md content i…

35536c5

…nstead of html content" This reverts commit 2598689.

Try to fix wikilink-embed e2e tests by resetting mocks in unit tests

c1eaded

Try to fix wikilink-embed e2e tests by deleting unit test

f86dc7e

Try to fix wikilink-embed e2e tests by removing extra await

7caac91

Try to fix wikilink-embed e2e tests by deleting the e2e tests to chec…

17baa5c

…k if there's a caching issue

Try to fix wikilink-embed e2e tests by recreating the e2e tests to ch…

f4e8575

…eck if there's a caching issue

Try to fix wikilink-embed e2e tests by restoring deleted spacing

2f5fe5e

badsketch added 3 commits August 22, 2023 22:38

Try to fix wikilink-embed e2e tests by removing embedNoteInContainer …

4f773f9

…config

Revert "Try to fix wikilink-embed e2e tests by removing embedNoteInCo…

24a6b59

…ntainer config" This reverts commit 4f773f9.

Try to fix wikilink-embed e2e tests by fixing the name of the config …

4190c43

…in the tests

badsketch added 2 commits August 22, 2023 23:36

Try to fix wikilink-embed e2e tests by initializing embedNoteInContai…

090d941

…ner in the tests

Revert "Try to fix wikilink-embed e2e tests by deleting unit test"

7b9c95d

This reverts commit f86dc7e.

Set embedNoteInContainer to null in e2e tests to more closely mimic l…

7365f5d

…ive state

riccardoferretti reviewed Aug 23, 2023

View reviewed changes

riccardoferretti approved these changes Aug 23, 2023

View reviewed changes

riccardoferretti merged commit 6c1d686 into foambubble:master Aug 23, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat #879: Different embedding styles prep #1273

Feat #879: Different embedding styles prep #1273

badsketch commented Aug 18, 2023

badsketch Aug 18, 2023

riccardoferretti Aug 18, 2023

badsketch Aug 18, 2023

riccardoferretti Aug 19, 2023

badsketch Aug 20, 2023

riccardoferretti Aug 20, 2023

badsketch Aug 20, 2023

riccardoferretti left a comment

riccardoferretti Aug 18, 2023

badsketch Aug 18, 2023

riccardoferretti Aug 19, 2023

badsketch Aug 20, 2023

riccardoferretti Aug 20, 2023

badsketch Aug 20, 2023

riccardoferretti Aug 18, 2023

riccardoferretti Aug 18, 2023

badsketch Aug 18, 2023

riccardoferretti Aug 19, 2023

badsketch Aug 20, 2023

riccardoferretti Aug 20, 2023

badsketch Aug 20, 2023

badsketch commented Aug 18, 2023

badsketch commented Aug 19, 2023

riccardoferretti commented Aug 20, 2023

badsketch commented Aug 20, 2023 •

edited

Loading

riccardoferretti left a comment

riccardoferretti Aug 21, 2023

badsketch Aug 22, 2023

riccardoferretti commented Aug 21, 2023

badsketch commented Aug 22, 2023 •

edited

Loading

riccardoferretti commented Aug 22, 2023

badsketch commented Aug 23, 2023

riccardoferretti Aug 23, 2023

riccardoferretti left a comment

Feat #879: Different embedding styles prep #1273

Feat #879: Different embedding styles prep #1273

Conversation

badsketch commented Aug 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riccardoferretti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

badsketch commented Aug 18, 2023

badsketch commented Aug 19, 2023

riccardoferretti commented Aug 20, 2023

badsketch commented Aug 20, 2023 • edited Loading

riccardoferretti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riccardoferretti commented Aug 21, 2023

badsketch commented Aug 22, 2023 • edited Loading

riccardoferretti commented Aug 22, 2023

badsketch commented Aug 23, 2023

Choose a reason for hiding this comment

riccardoferretti left a comment

Choose a reason for hiding this comment

badsketch commented Aug 20, 2023 •

edited

Loading

badsketch commented Aug 22, 2023 •

edited

Loading