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.

Sign up for GitHub

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

Jump to bottom

Switch back to gray-matter for front-matter parsing. #601

Merged
merged 5 commits into from
Sep 19, 2017
Merged

Switch back to gray-matter for front-matter parsing. #601

merged 5 commits into from
Sep 19, 2017

Conversation

tech4him1
Copy link
Contributor

@tech4him1 tech4him1 commented Sep 14, 2017 •
edited
Loading

- Summary
Switch from preliminaries back to gray-matter for front-matter parsing.

This is currently based on #592, and will be rebased after that one is merged. done

- Test plan
Passes current front-matter tests.

- Description for the changelog
Switch back to gray-matter for front-matter parsing.

@tech4him1 tech4him1 force-pushed the upgrade-gray-matter branch 2 times, most recently from afd871c to be05569 Compare September 14, 2017 18:20
@tech4him1 tech4him1 changed the title WIP: Switch back to gray-matter for front-matter parsing. Switch back to gray-matter for front-matter parsing. Sep 14, 2017
@tech4him1 tech4him1 requested review from erquhart and Benaiah and removed request for Benaiah and erquhart September 14, 2017 19:24
@tech4him1 tech4him1 changed the title Switch back to gray-matter for front-matter parsing. WIP: Switch back to gray-matter for front-matter parsing. Sep 14, 2017
@tech4him1 tech4him1 changed the title WIP: Switch back to gray-matter for front-matter parsing. Switch back to gray-matter for front-matter parsing. Sep 14, 2017
@tech4him1 tech4him1 requested a review from Benaiah September 14, 2017 22:59
@tech4him1 tech4him1 mentioned this pull request Sep 14, 2017
Closed
@tech4him1 tech4him1 changed the title Switch back to gray-matter for front-matter parsing. WIP: Switch back to gray-matter for front-matter parsing. Sep 15, 2017
@tech4him1 tech4him1 changed the title WIP: Switch back to gray-matter for front-matter parsing. Switch back to gray-matter for front-matter parsing. Sep 15, 2017
erquhart
erquhart reviewed Sep 15, 2017
View reviewed changes
src/formats/frontmatter.js
toml: tomlEng.parse.bind(tomlEng),
json: (input) => {
let JSONinput = input.trim();
// Fix JSON if leading and trailing brackets were trimmed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would brackets be trimmed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delimiters always get trimmed. For stuff like YAML where the delims are --- and not actually part of the code that is helpful. But with JSON, the delimiters { and } get trimmed, but they actually need to be in there for the parser. With preliminaries they did this too, here: preliminaries/index.js#L376-L379

We can't just add them all the time, though, we have to if-check first, because they could have wrapped the whole thing in a ---json delimiter, in which case the brackets wouldn't get trimmed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also see the difference in the tests:
https://github.com/netlify/netlify-cms/blob/3195f30b8d174b4820ef65d07d64ded336477af8/src/formats/__tests__/frontmatter.spec.js#L69
vs
https://github.com/netlify/netlify-cms/blob/3195f30b8d174b4820ef65d07d64ded336477af8/src/formats/__tests__/frontmatter.spec.js#L81

@tech4him1 tech4him1 requested a review from erquhart September 16, 2017 00:37
Benaiah
Benaiah approved these changes Sep 18, 2017
View reviewed changes
Copy link
Contributor

@Benaiah Benaiah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

erquhart
erquhart approved these changes Sep 19, 2017
View reviewed changes
Copy link
Contributor

@erquhart erquhart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@erquhart erquhart merged commit 1def72e into decaporg:master Sep 19, 2017
@tech4him1 tech4him1 deleted the upgrade-gray-matter branch September 19, 2017 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erquhart erquhart erquhart approved these changes

@Benaiah Benaiah Benaiah approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tech4him1 @Benaiah @erquhart