Add conversion of FrontMatterDocument to raw String #2
Conversation
Added * Add conversion of FrontMatterDocument to front-matter markdown files Modified * Add .idea/ to .gitignore * Reimplement parseContent and parseFile functions as constructors/functions on FrontMatterDocument * Change FrontMatterDocument.value to a getter transforming content and data to a front-matter markdown files * Update test to match changes * Update example to match changes
|
Unfortunately, the YAML spec forbids parsers from keeping key/value definition order, so, to keep orders consistent, the raw generation currently sorts entries by comparing the keys. |
|
Thanks for your PR @MaterialTime. Your formatter is adding spaces to function parentheses. While I take a deeper look at your PR, would you mind pushing a commit to revert those changes to reduce the diff a bit? I'm using the default |
|
@izolate Yeah, sorry about that. I'd be happy to push a commit without those changes. |
| var fileContents = await file.readAsString(); | ||
|
|
||
| var doc = front_matter.parse(fileContents); | ||
| var doc = FrontMatterDocument.fromText(fileContents); |
There was a problem hiding this comment.
Was this change really necessary? It introduces a breaking API change. Happy to discuss this in a separate issue (or PR) but I think this is the wrong PR for it.
There was a problem hiding this comment.
If you feel this PR isn't the place for it, I can revert it back to the way it is done in your version of the package.
There was a problem hiding this comment.
If you don't mind me asking, as I'm working on reverting it now, is there a reason you have the parse method pointing directly to the parseContent method, except for the defaultDelimiter bit?
Would you mind if I just put the contents of parseContent directly into the parse method?
There was a problem hiding this comment.
is there a reason you have the
parsemethod pointing directly to theparseContentmethod?
Just a functional programming style.
Would you mind if I just put the contents of
parseContentdirectly into theparsemethod?
I would, unfortunately. I've just pushed a new release 1.1.0 with some small changes. One of the changes is to rename the parseContent method to parser. I'd appreciate if you kept the current directory structure, with the two methods (parse and parseFile) exposed from front_matter.dart delegating out to the parser() from parser.dart.
I recommend rebasing with the latest commits from master and starting from there.
There was a problem hiding this comment.
While I'm considering how to implement my changes in line with the things you've mentioned, is the FrontMatterDocument constructor supposed to be public? Is it supposed to be used outside of the package itself?
There was a problem hiding this comment.
Is it supposed to be used outside of the package itself?
At the moment, no. But this is subject to change.
There was a problem hiding this comment.
@MaterialTime the more I think about it, the less I'm convinced that this FrontMatterDocument to String method is possible. The method to generate the YAML string from a Map is outside the scope of this package, and frankly better suited to the yaml package.
There was a problem hiding this comment.
I've looked around, and the yamlicious package might be a suitable alternative for generating the YAML portion of value. I hope that this can be used to resolve the issue of generating YAML strings being outside the scope of this package.
| // The content begins after the closing delimiter index. | ||
| content = value.substring(closeIndex + (delimiter.length + 1)).trim(); | ||
|
|
||
| print('content: $content'); |
There was a problem hiding this comment.
Why return the content: prefix here? Just return content directly.
There was a problem hiding this comment.
Also, what's your reasoning for using print() here?
There was a problem hiding this comment.
Sorry about that. I believe that was there for testing and I forgot to remove it when I pushed the commit.
| YamlMap data; | ||
|
|
||
| FrontMatterDocument(this.value); | ||
| String get value { |
There was a problem hiding this comment.
Presuming this method was the reason for the PR, the value getter already exists in the latest version of this package. You can access the original raw value like so:
var str = "---\nfoo: bar\n---\nHello, world!";
var doc = fm.parse(str);
assert(doc.value, equals(str)); // trueReturning the original value directly is preferable over generating the raw string from the parsed document, to avoid any discrepancies.
There was a problem hiding this comment.
The reason for the getter is so that the document can be modified and converted back to a raw String to be stored. Eg. I'm planning to use it with Git storage.
There was a problem hiding this comment.
I've just released 1.1.0 with a toString() method on FrontMatterDocument that returns the raw, original value. Is this what you need?
Perhaps we should pause this PR and open the discussion in the issue #1 ?
There was a problem hiding this comment.
No. Maybe I'm communicating it wrong, but the point of the getter is so that one can modify the data or content variables on a FrontMatterDocument can be modified and easily converted back to a raw front-matter document.
2 participants
This pull request proposes adding support for converting FrontMatterDocument to a raw String.
See #1 for the initial discussion.