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.

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

Break on unsupported file format. #797

Closed
tech4him1 opened this issue Nov 10, 2017 · 13 comments · Fixed by #831
Closed

Break on unsupported file format. #797

tech4him1 opened this issue Nov 10, 2017 · 13 comments · Fixed by #831
Assignees

Comments

@tech4him1
Copy link
Contributor

tech4him1 commented Nov 10, 2017

Currently, if an unsupported file format is set on a collection (format: ini), we simply assume that it should be saved as YAML front-matter (see #748 and #776). I think in this case, when the user has explicitly set a format, and we are not inferring at all, it makes more sense to just throw an "Invalid Config" error. @erquhart @Benaiah?

Expanded discussion: #776 (comment)

@tech4him1
Copy link
Contributor Author

We could throw on extension as well, but there may be a valid use case where someone needs to have a different extension than the normal one for a filetype. It does make it hader for the user to know that we are inferring, though.

@Benaiah
Copy link
Contributor

Benaiah commented Nov 10, 2017

I think throwing on an unsupported format is a good idea. I think we should also throw if the extension is unsupported and there isn't a valid format set. We shouldn't error if there is an unknown extension but a valid format.

@tech4him1
Copy link
Contributor Author

Good point, then if people want to force an abnormal extension, they have to explicitly set the format.

@tech4him1 tech4him1 self-assigned this Nov 13, 2017
@tech4him1
Copy link
Contributor Author

Current status (after #796):

format extension File Formatter
md md
known known known
unknown md md Should throw.
known known known
unknown undefined md Broken + Should throw.
unknown known md md Should throw.
unknown unknown unknown md Should throw.

@tech4him1
Copy link
Contributor Author

@Benaiah @erquhart Or should we not even try to infer from the extension at all -- just throw if a format is not set along with the extension?

@erquhart
Copy link
Contributor

erquhart commented Nov 28, 2017

@tech4him1 nice table! This conversation is happening in a few places, is there a canonical issue or PR that we can condense down to?

@tech4him1
Copy link
Contributor Author

Sure, most of the discussion here relates to #776, I'll move it there.

@tech4him1
Copy link
Contributor Author

The core of this issue is still just throwing on an unsupported format, so I'll leave it open until that gets merged. I moved the other discussion to #776.

@tech4him1 tech4him1 reopened this Nov 28, 2017
@tech4him1
Copy link
Contributor Author

tech4him1 commented Dec 1, 2017

@Benaiah @erquhart Should we account for "common misspellings" on the formats? yml vs yaml, md vs markdown? Right now it's really weird -- we account for the misspellings, but the extension is saved as undefined. We either need to throw on misspellings and provide a list of supported options, or just accept them as normal options.

@Benaiah
Copy link
Contributor

Benaiah commented Dec 1, 2017

@tech4him1 Yes, I think those alternative format names should be aliases for the ones we use.

@erquhart
Copy link
Contributor

erquhart commented Dec 1, 2017

We just can't change the extension on existing entries (don't want to bother with the tree manipulation to produce a rename). But yeah, that should work.

@tech4him1
Copy link
Contributor Author

@erquhart What about the format: frontmatter option? https://github.com/netlify/netlify-cms/blob/2ecc5355d9fe55c84e78f5f315d6b822dfbfa0de/src/formats/formats.js#L43 Is there a reason that needs be be in there, instead of using format: markdown, since right now the default file extension for it is undefined?

@erquhart
Copy link
Contributor

erquhart commented Dec 1, 2017

If we don't need it, let's drop it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants