Custom delimiters #1064
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,9 @@ | ||
| import yamlFormatter from './yaml'; | ||
| import tomlFormatter from './toml'; | ||
| import jsonFormatter from './json'; | ||
| import { FrontmatterInfer, frontmatterJSON, frontmatterTOML, frontmatterYAML } from './frontmatter'; | ||
|
|
||
| export const frontmatterFormats = ['yaml-frontmatter','toml-frontmatter','json-frontmatter'] | ||
|
|
||
| export const supportedFormats = [ | ||
| 'yml', | ||
|
|
@@ -37,24 +39,26 @@ export function formatByExtension(extension) { | |
| }[extension]; | ||
| } | ||
|
|
||
| function formatByName(name, customDelimiter) { | ||
| return { | ||
| yml: yamlFormatter, | ||
| yaml: yamlFormatter, | ||
| toml: tomlFormatter, | ||
| json: jsonFormatter, | ||
| frontmatter: FrontmatterInfer, | ||
| 'json-frontmatter': frontmatterJSON(customDelimiter), | ||
| 'toml-frontmatter': frontmatterTOML(customDelimiter), | ||
| 'yaml-frontmatter': frontmatterYAML(customDelimiter), | ||
| }[name]; | ||
| } | ||
|
|
||
| export function resolveFormat(collectionOrEntity, entry) { | ||
| // Check for custom delimiter | ||
| const customDelimiter = collectionOrEntity.get('frontmatter_delimiter'); | ||
|
There was a problem hiding this comment. I think we need to throw if a There was a problem hiding this comment. @Swieckowski I think we discussed this before, was there something that I missed? There was a problem hiding this comment. Ah yes, I remember bringing that up myself but don't recall a specific response, sorry. Should I get on that asap? There was a problem hiding this comment. No problem! I'd like to have it in before merge -- if you're busy, I can do it. There was a problem hiding this comment. I can have it done by 2 or 3pm tomorrow if that's alright. There was a problem hiding this comment. Oh, no hurry at all, thank you! There was a problem hiding this comment. @tech4him1 I've been a bit swamped with job search stuff haha, but I hope my latest commits address what you were talking about. We now throw an error if someone has a frontmatter_delimiter set without setting their format or not setting it to the proper frontmatter formats. There was a problem hiding this comment. Also, since I've added tests and documentation, how do I add that to my contribution? The add contributor script is failing on me, perhaps because I've already run it once on my github username? |
||
| // If the format is specified in the collection, use that format. | ||
| const formatSpecification = collectionOrEntity.get('format'); | ||
| if (formatSpecification) { | ||
| return formatByName(formatSpecification, customDelimiter); | ||
| } | ||
|
|
||
| // If a file already exists, infer the format from its file extension. | ||
|
|
@@ -72,5 +76,5 @@ export function resolveFormat(collectionOrEntity, entry) { | |
| } | ||
|
|
||
| // If no format is specified and it cannot be inferred, return the default. | ||
| return formatByName('frontmatter', customDelimiter); | ||
| } | ||
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.
I do wonder if there is a better way to avoid the overhead that is incurred here. The benefit of the single objects that we had before was that there was only a single instance of each created for the entire CMS (7 total instances). Now, we're creating three new objects for each page, then, throwing them all away (unless one is used). That just seems a little wasteful to me.
To pass a custom parameter, we're obviously going to have to do more than one instance. I think the cleanest, most sustainable way would be to create only one instance per collection.
For now, I wonder if we could just wait to pass the
customDelimiterin until afterformatByNamereturns the function. I haven't thought this through that much, though.@erquhart Do you think this could cause performance problems now, or would you like to merge this, and change it later? I haven't worked with JS enough to know how expensive this really is.
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.
I doubt any real performance penalty would happen here. I did have the same thoughts on improvements, but trying to prioritize timely merges over ideal code. This passed the "good enough" test for me, personally.
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 because I'm passing in customDelimiters here, you're saying that each object gets actually instantiated while before it did not? That totally escaped me, I can test out a few things and make this more optimized if you'd like.
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.
@Swieckowski The difference is that a new object gets created with each function call, instead of just reusing the same one. I think, like Shawn said, that it's probably not worth delaying the PR just to try to optimize the code. I don't really want to delay it just for that, and we can optimize later if necessary. Do you agree?
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.
Sure! I was just trying to make sure I was following your logic correctly.