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

Collections with some custom extensions break. #776

Closed
tech4him1 opened this issue Nov 5, 2017 · 14 comments · Fixed by #796
Closed

Collections with some custom extensions break. #776

tech4him1 opened this issue Nov 5, 2017 · 14 comments · Fixed by #796

Comments

@tech4him1
Copy link
Contributor

tech4him1 commented Nov 5, 2017

- Do you want to request a feature or report a bug?
bug, related to #763

- What is the current behavior?
If a custom file extension is set in the collection config, and it is one of the ones in /src/formats/formats.js: formatByExtension(), the file will be saved as frontmatter. But the format of the file will try to be inferred when reading, so the file cannot be read.

- If the current behavior is a bug, please provide the steps to reproduce.

  1. Create a collection with the setting extension: json. Do not set a format.
  2. Create (and save) an entry in that collection. Notice it is saved as frontmatter/markdown.
  3. Try to open that entry/collection. The file cannot be opened, as the CMS tries to read it as JSON.

- What is the expected behavior?
The behavior should be consistent between reads and writes. If we try to infer the format while reading we should try to infer it while writing as well, or infer neither time.

Discussion Summary: #776 (comment)

- Please mention your CMS, node.js, and operating system version.

CMS 0.6.0

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

@erquhart @Benaiah @verythorough Any thoughts?

@verythorough
Copy link
Contributor

I think it makes sense that one should imply the other, since in the vast majority of cases, they will be the same. On the implementer side, I don't think it really matters which of the two is the "implied" field, so if "let format follow extension" makes the most sense in the code, that works for me. (Of course, if format is explicitly set, that setting should override any inference.)

@erquhart
Copy link
Contributor

erquhart commented Nov 7, 2017

Thoughts:

  • format should only set the frontmatter format for markdown files (and eventually others like hbs) - for other files, format should be ignored
  • If extension is something other than empty or md/markdown, format should be ignored
  • If either format or extension contains a value that we don't recognize, throw an error
  • At some point, we should rename format to something like frontmatterType or frontmatterFormat

Thoughts?

@Benaiah
Copy link
Contributor

Benaiah commented Nov 7, 2017

@erquhart that would prevent you from having a folder of data files with a custom extension - for instance, a folder of .blogpost files that contain json. I think we should have format define the format for the whole file (json/yaml/toml/etc), have it override the value we infer from extension if format is set, and create a separate option frontmatterFormat for specifying frontmatter formats.

@tech4him1
Copy link
Contributor Author

@erquhart I think that what you are discussing is slightly different than what I am in this issue. In this issue specifically, I'm just wondering whether we should try to infer the format of new entries when extension is set, or whether the user should have to set format as well (we should not to to infer it from the extension).

@erquhart
Copy link
Contributor

@tech4him1:

If extension is something other than empty or md/markdown, format should be ignored

@tech4him1
Copy link
Contributor Author

Still, I'm not talking about front-matter format, but the actual file format (JSON, YAML, MD, TOML).

@erquhart
Copy link
Contributor

Yeah I was thinking frontmatter. So assuming the extension is something that makes sense, we always infer, but @Benaiah is right, we need to allow folks to trump all by setting both, and split between format and frontmatter format.

@tech4him1
Copy link
Contributor Author

tech4him1 commented Nov 28, 2017

Quoting @Benaiah from #797 (comment):

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

Status if we start inferring format from extension:

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.

@erquhart
Copy link
Contributor

erquhart commented Nov 29, 2017

So we do indeed need to add a formatFrontmatter config option, that's one thing.

For some reason that table, although lovely, is hard for me to parse just looking at it, so let me know if we're on the same page - here's what I'd expect for file formats:

  • First of all, we always throw if the format is unrecognized, regardless of how format is determined.
  • If neither is set, we use the default format and extension.
  • If only format is set (and valid), we use the default extension for that format.
  • If only extension is set (and valid), we infer format.
  • If both are set, and format is valid, we use the provided extension.

Anything missing here?

@tech4him1
Copy link
Contributor Author

Your list is what we are doing for writes, the order for reads is also an issue here:

  • If only format is set (and valid), we use the default extension for that format.
  • If neither is set, we infer the format from the file extension.
  • The extension setting doesn't matter at all here.

Also, when reading, we filter out every file that does not match the extension that we are using (inferred or manually set).

Agreed?

@erquhart
Copy link
Contributor

If neither is set, we infer the format from the file extension.

The problem here is what to do if there are multiple extensions. If there are, there's no great way to pick. For example, we could go with whichever is most prevalent, but that balance can change, and suddenly the user sees a different set of files in the collection. We could also only infer from extension if all files have the same extension, but that means checking every file extension, which won't work when/if pagination is a factor.

Can you see any other way? Otherwise I'm thinking the rules I wrote apply for both read and write, and we can still infer format from extension on read.

@tech4him1
Copy link
Contributor Author

Ahh, you are correct there, since we filter out everything that doesn't match the extension, and the extension is set by default to markdown. I think your list is correct, then, for both.

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.

4 participants