-
Notifications
You must be signed in to change notification settings - Fork 662
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
Feat #879: Different embedding styles prep #1273
Changes from 1 commit
4e2f07a
e05c1c2
33e4438
e3fe400
3b1c903
7d97e78
2598689
35536c5
c1eaded
f86dc7e
7caac91
17baa5c
f4e8575
2f5fe5e
4f773f9
24a6b59
4190c43
090d941
7b9c95d
7365f5d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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( | ||
CONFIG_EMBED_NOTE_IN_CONTAINER | ||
) | ||
? 'card' | ||
: 'inline'; | ||
const noteEmbedder = new NoteEmbedder( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A little confused by "encapsulate a behavior". Is that a type of implementation pattern? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no no, what I mean is basically that currently in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about moving the two lines of 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 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. As an aside, the functional way of mimicking the constructor would be using a factory, e.g.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great suggestion, done! Also really appreciate the snippet |
||
includedNote, | ||
includedNote.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, | ||
'full', | ||
noteStyle, | ||
parser, | ||
workspace | ||
workspace, | ||
md | ||
); | ||
content = getFoamVsCodeConfig(CONFIG_EMBED_NOTE_IN_CONTAINER) | ||
? `<div class="embed-container-note">${md.render(noteText)}</div>` | ||
: noteText; | ||
content = noteEmbedder.generateEmbedding(); | ||
break; | ||
} | ||
case 'attachment': | ||
|
@@ -123,4 +118,102 @@ function withLinksRelativeToWorkspaceRoot( | |
return text; | ||
} | ||
|
||
interface EmbedNoteExtractor { | ||
extract(note: Resource): string; | ||
} | ||
|
||
class FullExtractor implements EmbedNoteExtractor { | ||
parser: ResourceParser; | ||
workspace: FoamWorkspace; | ||
|
||
constructor(parser: ResourceParser, workspace: FoamWorkspace) { | ||
this.parser = parser; | ||
this.workspace = workspace; | ||
} | ||
|
||
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; | ||
} | ||
} | ||
|
||
badsketch marked this conversation as resolved.
Show resolved
Hide resolved
|
||
interface EmbedNoteFormatter { | ||
format(content: string): string; | ||
} | ||
|
||
class CardFormatter implements EmbedNoteFormatter { | ||
md: markdownit; | ||
constructor(md: markdownit) { | ||
this.md = md; | ||
} | ||
badsketch marked this conversation as resolved.
Show resolved
Hide resolved
|
||
format(content: string) { | ||
return `<div class="embed-container-note">${this.md.render(content)}</div>`; | ||
} | ||
} | ||
|
||
class InlineFormatter implements EmbedNoteFormatter { | ||
format(content: string) { | ||
return content; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you still wanna render here right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the switch statement for note, attachment, and image all return content and outside the switch statement 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the issue is that card style needs an additional 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;
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I see what you mean, I had forgotten about the double rendering. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
} | ||
} | ||
|
||
class NoteEmbedder { | ||
includedNote: Resource; | ||
extractor: EmbedNoteExtractor; | ||
formatter: EmbedNoteFormatter; | ||
|
||
/* extractor dependencies */ | ||
parser: ResourceParser; | ||
workspace: FoamWorkspace; | ||
|
||
/* formatter dependencies */ | ||
md: markdownit; | ||
|
||
constructor( | ||
includedNote: Resource, | ||
extractorType: string, | ||
formatterType: string, | ||
parser: ResourceParser, | ||
workspace: FoamWorkspace, | ||
md: markdownit | ||
) { | ||
this.includedNote = includedNote; | ||
|
||
switch (extractorType) { | ||
case 'full': | ||
case 'content': // TODO: IMPLEMENT | ||
default: | ||
this.extractor = new FullExtractor(parser, workspace); | ||
break; | ||
} | ||
|
||
switch (formatterType) { | ||
case 'card': | ||
this.formatter = new CardFormatter(md); | ||
break; | ||
case 'inline': | ||
default: | ||
this.formatter = new InlineFormatter(); | ||
break; | ||
} | ||
} | ||
|
||
generateEmbedding() { | ||
const rawContent = this.extractor.extract(this.includedNote); | ||
return this.formatter.format(rawContent); | ||
} | ||
} | ||
|
||
export default markdownItWikilinkEmbed; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the next phase, I'd update the regex to capture the new syntax. Then here we'd have logic to determine
noteStyle
andnoteContent
. For now,noteStyle
pulls from the existing config setting, and there is nonoteContent
, it just defaults to "full".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct, although from the configuration we should be getting both the style and the content, is that planned for this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 overridespreview.embedNoteInContainer
if both are set. I can also add a deprecation flag topreview.embedNoteInContainer
so it no longer appears in settings UI. In the next PR, I can remove it altogetherpackage.json
as well as this file. wdyt?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean for the deprecation flag? https://code.visualstudio.com/api/references/contribution-points#contributes.configuration:~:text=deprecationMessage%20/%20markdownDeprecationMessage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's great, I was already thinking of a small utility to deal with deprecation, but that won't be necessary 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sweet!