Custom delimiters #1064

Swieckowski · 2018-01-31T02:26:14Z

- Summary

This allows users to specify their own custom delimiters for frontmatter if they've specified a format type. Closes #980

- Test plan

I checked all the different types of format settings with unique delimiters. Not sure if I could have done more.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

verythorough · 2018-01-31T02:27:54Z

Deploy preview for netlify-cms-www ready!

Built with commit 98b33c9

https://deploy-preview-1064--netlify-cms-www.netlify.com

verythorough · 2018-01-31T02:27:56Z

Deploy preview for cms-demo ready!

Built with commit 98b33c9

https://deploy-preview-1064--cms-demo.netlify.com

erquhart · 2018-02-01T17:54:10Z

Thanks @Swieckowski! Going to take a look at this soon.

Swieckowski · 2018-02-01T22:55:00Z

Good to hear @erquhart, hopefully I can do a PR on my other issue claim soon enough as well.

erquhart

LGTM - thanks @Swieckowski!

tech4him1 · 2018-02-03T02:58:58Z

I'll review ASAP.

tech4him1

Thanks @Swieckowski!

tech4him1 · 2018-02-06T20:25:17Z

src/formats/formats.js

-    'yaml-frontmatter': FrontmatterYAML,
+    'json-frontmatter': frontmatterJSON(customDelimiter),
+    'toml-frontmatter': frontmatterTOML(customDelimiter),
+    'yaml-frontmatter': frontmatterYAML(customDelimiter),


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 customDelimiter in until after formatByName returns 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.

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.

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.

@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?

Sure! I was just trying to make sure I was following your logic correctly.

tech4him1 · 2018-02-06T20:25:22Z

src/formats/formats.js

  }

  // If a file already exists, infer the format from its file extension.
  const filePath = entry && entry.path;
  if (filePath) {
    const fileExtension = filePath.split('.').pop();
-    return formatByExtension(fileExtension);
+    return formatByExtension(fileExtension, customDelimiter);


I don't think we need to pass customDelimiter to formatByExtension() when it doesn't use it. That can always be added later if needed.

I just removed the unnecessary passing of customDelimiter to formatByExtension(), it was a remnant of a different train of thought. Sorry I didn't have it cleaned up before.

Are there any other steps I should take with this?

Nope, thank you.

tech4him1 · 2018-02-06T20:27:51Z

src/formats/formats.js

  }[name];
 }

 export function resolveFormat(collectionOrEntity, entry) {
+  // Check for custom delimiter
+  const customDelimiter = collectionOrEntity.get('frontmatter_delimiter');


I think we need to throw if a frontmatter_delimiter is set without an explicit format being set. This might be better checked in the reducer config validation section (/src/reducers/collections.js).

@Swieckowski I think we discussed this before, was there something that I missed?

Ah yes, I remember bringing that up myself but don't recall a specific response, sorry. Should I get on that asap?

No problem! I'd like to have it in before merge -- if you're busy, I can do it.

I can have it done by 2 or 3pm tomorrow if that's alright.

Oh, no hurry at all, thank you!

@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.

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?

erquhart · 2018-02-07T22:35:11Z

Eww resolving conflicts in GitHub's UI results in a merge commit 🤦‍♂️ .

Anywho, feel free to merge when you're happy with this @tech4him1. Thanks again @Swieckowski!

verythorough · 2018-02-08T12:43:50Z

website/site/content/docs/configuration-options.md

-The explicit `yaml-frontmatter`, `toml-frontmatter`, and `json-frontmatter` formats above do not currently support custom delimiters. We use `---` for YAML, `+++` for TOML, and `{` `}` for JSON. If a file has frontmatter inside other delimiters it will be included as part of the body text.
+The explicit `yaml-frontmatter`, `toml-frontmatter`, and `json-frontmatter` formats support custom delimiters. We use `---` for YAML, `+++` for TOML, and `{` `}` for JSON by default, but you can configure a custom delimiter.
+You can configure a custom delimiter with `frontmatter_delimiter`, but it only applies if you have a frontmatter format specified.
+If a file has frontmatter inside other delimiters it will be included as part of the body text.


Thanks for including this change in the docs!

Could you format this like the other config options? Something like:

frontmatter_delimiter: description here

I think you could also add the default delimiters (+++ etc) to the descriptions for the explicit frontmatter formats instead of putting them all together in the frontmatter_delimiter description.

@verythorough I took your comments into consideration, I hope the changes reflect the direction you were looking for.

…atter format

…lify-cms into customDelimiters

…tmatter format being declared

tech4him1 · 2018-02-09T00:26:10Z

src/reducers/collections.js

@@ -38,6 +38,12 @@ function validateCollection(configCollection) {
    // Cannot infer format from extension.
    throw new Error(`Please set a format for collection "${ collectionName }". Supported formats are ${ supportedFormats.join(',') }`);
  }
+  const frontmatterFormats = ['yaml-frontmatter','toml-frontmatter','json-frontmatter']


I'd rather have this list imported from src/formats/formats.js, like we did for the supportedFormats one above. Just easier to keep it together.

Fixed. I will think more carefully about where I place things from now, thanks for your patience.

Oh no, not a problem at all. Sorry to bug you with little things.

tech4him1 · 2018-02-09T00:41:04Z

@Swieckowski Looks like the rebase messed up the all contributors, you should be able to run it again and add tests and docs.

Swieckowski · 2018-02-09T00:55:01Z

@tech4him1 I think I've addressed all of your concerns except for the minor optimization. Which I could slightly ameliorate with a switch statement refactor instead of the object property approach currently used, but I don't think the derailment from the norm is worth the slight optimization (which wouldn't bring the performance back to the same level anyway).

The community's been really helpful, and I think my next PR will benefit from all the input I've received.

Swieckowski added 4 commits January 30, 2018 15:03

frontmatter functions now exported as curried constructors

ca559ec

changed formats.js to accomodate frontmatter.js changes

fe1368c

can only use custom delimiters if format is specified

82ba3f3

reverting changes

50a5ed0

Swieckowski closed this Jan 31, 2018

Swieckowski reopened this Jan 31, 2018

Swieckowski closed this Jan 31, 2018

Swieckowski added 2 commits January 30, 2018 21:52

updated tests to reflect changes

8a32024

created tests for custom delimiters

1450c8c

Swieckowski reopened this Jan 31, 2018

tech4him1 added impact: changelog impact: configuration status: 1-design-review area: formats labels Jan 31, 2018

updated docs to reflect addition of custom delimiter configuration

67c02fd

tech4him1 requested review from tech4him1 and erquhart February 1, 2018 17:56

erquhart approved these changes Feb 2, 2018

View reviewed changes

tech4him1 suggested changes Feb 6, 2018

View reviewed changes

Swieckowski and others added 3 commits February 6, 2018 15:46

formatByExtension no longer takes custom delimiter

6ec09aa

missed some formatByExtension calls

d56fda8

Merge branch 'master' into customDelimiters

0155db0

verythorough reviewed Feb 8, 2018

View reviewed changes

Edited the docs according to suggestions

b680d3d

Swieckowski added 4 commits February 8, 2018 16:57

Throws an error if a custom delimiter is set without declaring frontm…

5ed2040

…atter format

Merge branch 'customDelimiters' of https://github.com/Swieckowski/net…

5549972

…lify-cms into customDelimiters

Throws error if custom delimiter set without explicit and proper fron…

8063b96

…tmatter format being declared

example config file set to proper defaults

b25d11e

tech4him1 reviewed Feb 9, 2018

View reviewed changes

Swieckowski and others added 2 commits February 8, 2018 19:29

moved frontmatterFormats to formats

64433de

Update .all-contributorsrc

b6072f0

Swieckowski and others added 2 commits February 8, 2018 19:43

updated contributions

e0d5301

Clarify docs

98b33c9

tech4him1 approved these changes Feb 9, 2018

View reviewed changes

tech4him1 merged commit afce6ab into decaporg:master Feb 9, 2018

tech4him1 mentioned this pull request Feb 9, 2018

Allow different beginning and ending frontmatter delimiters. #1094

Merged

daneden mentioned this pull request Jan 8, 2019

Update configSchema.js to allow arrays for frontmatter_delimiter #1997

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom delimiters #1064

Custom delimiters #1064

Swieckowski commented Jan 31, 2018 •

edited

Loading

verythorough commented Jan 31, 2018 •

edited

Loading

verythorough commented Jan 31, 2018 •

edited

Loading

erquhart commented Feb 1, 2018

Swieckowski commented Feb 1, 2018

erquhart left a comment

tech4him1 commented Feb 3, 2018

tech4him1 left a comment

tech4him1 Feb 6, 2018

erquhart Feb 6, 2018

Swieckowski Feb 6, 2018

tech4him1 Feb 6, 2018 •

edited

Loading

Swieckowski Feb 6, 2018

tech4him1 Feb 6, 2018

Swieckowski Feb 6, 2018

tech4him1 Feb 6, 2018

tech4him1 Feb 6, 2018 •

edited

Loading

tech4him1 Feb 6, 2018

Swieckowski Feb 6, 2018

tech4him1 Feb 6, 2018 •

edited

Loading

Swieckowski Feb 6, 2018

tech4him1 Feb 6, 2018

Swieckowski Feb 8, 2018

Swieckowski Feb 8, 2018

erquhart commented Feb 7, 2018

verythorough Feb 8, 2018

Swieckowski Feb 8, 2018

tech4him1 Feb 9, 2018

Swieckowski Feb 9, 2018

tech4him1 Feb 9, 2018

tech4him1 commented Feb 9, 2018 •

edited

Loading

Swieckowski commented Feb 9, 2018

Custom delimiters #1064

Custom delimiters #1064

Conversation

Swieckowski commented Jan 31, 2018 • edited Loading

verythorough commented Jan 31, 2018 • edited Loading

verythorough commented Jan 31, 2018 • edited Loading

erquhart commented Feb 1, 2018

Swieckowski commented Feb 1, 2018

erquhart left a comment

Choose a reason for hiding this comment

tech4him1 commented Feb 3, 2018

tech4him1 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

tech4him1 Feb 6, 2018 • edited Loading

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

tech4him1 Feb 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tech4him1 Feb 6, 2018 • edited Loading

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

erquhart commented Feb 7, 2018

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

tech4him1 commented Feb 9, 2018 • edited Loading

Swieckowski commented Feb 9, 2018

Swieckowski commented Jan 31, 2018 •

edited

Loading

verythorough commented Jan 31, 2018 •

edited

Loading

verythorough commented Jan 31, 2018 •

edited

Loading

tech4him1 Feb 6, 2018 •

edited

Loading

tech4him1 Feb 6, 2018 •

edited

Loading

tech4him1 Feb 6, 2018 •

edited

Loading

tech4him1 commented Feb 9, 2018 •

edited

Loading