-
Notifications
You must be signed in to change notification settings - Fork 6
Add conversion of FrontMatterDocument to raw String #2
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,85 @@ | ||
| import 'dart:io'; | ||
|
|
||
| import 'package:meta/meta.dart'; | ||
| import 'package:yaml/yaml.dart'; | ||
|
|
||
| import '../front_matter.dart'; | ||
|
|
||
| // Default delimiter for YAML. | ||
| const defaultDelimiter = '---'; | ||
|
|
||
| /// Document containing the original `value`, front matter `data` and `content`. | ||
| class FrontMatterDocument { | ||
| String value, content; | ||
| FrontMatterDocument({@required this.data, @required this.content}); | ||
|
|
||
| FrontMatterDocument.fromText(String text, | ||
| {String delimiter = defaultDelimiter}) { | ||
| // Remove any leading whitespace. | ||
| final value = text.trimLeft(); | ||
|
|
||
| // If there's no starting delimiter, there's no front matter. | ||
| if (!value.startsWith(delimiter)) { | ||
| content = value; | ||
| data = YamlMap(); | ||
| return; | ||
| } | ||
|
|
||
| // Get the index of the closing delimiter. | ||
| final closeIndex = value.indexOf('\n' + delimiter); | ||
|
|
||
| // Get the raw front matter block between the opening and closing delimiters. | ||
| final frontMatter = value.substring(delimiter.length, closeIndex); | ||
|
|
||
| if (frontMatter.isNotEmpty) { | ||
| try { | ||
| // Parse the front matter as YAML. | ||
| data = loadYaml(frontMatter); | ||
| } catch (e) { | ||
| throw FrontMatterException(invalidYamlError); | ||
| } | ||
| } | ||
|
|
||
| // The content begins after the closing delimiter index. | ||
| content = value.substring(closeIndex + (delimiter.length + 1)).trim(); | ||
|
|
||
| print('content: $content'); | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why return the
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, what's your reasoning for using
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry about that. I believe that was there for testing and I forgot to remove it when I pushed the commit. |
||
| } | ||
|
|
||
| static fromFile(String path, {String delimiter = defaultDelimiter}) async { | ||
| var file = File(path); | ||
|
|
||
| // Throw an error if file not found. | ||
| if (!await file.exists()) { | ||
| throw FrontMatterException(fileNotFoundError); | ||
| } | ||
|
|
||
| try { | ||
| var text = await file.readAsString(); | ||
| return FrontMatterDocument.fromText(text, delimiter: delimiter); | ||
| } catch (e) { | ||
| // Handle downstream errors, or throw one if file is not readable as text. | ||
| switch (e.message) { | ||
| case invalidYamlError: | ||
| rethrow; | ||
| default: | ||
| throw FrontMatterException(fileTypeError); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| String content; | ||
|
|
||
| /// The parsed YAML front matter as a [YamlMap]. | ||
| YamlMap data; | ||
|
|
||
| FrontMatterDocument(this.value); | ||
| String get value { | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Presuming this method was the reason for the PR, the 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've just released Perhaps we should pause this PR and open the discussion in the issue #1 ?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. Maybe I'm communicating it wrong, but the point of the getter is so that one can modify the |
||
| var newValue = '---\n'; | ||
|
|
||
| for (final d in data.entries.toList() | ||
| ..sort((a, b) => a.key.compareTo(b.key))) { | ||
| newValue += '${d.key}: ${d.value}\n'; | ||
| } | ||
|
|
||
| return '$newValue---\n\n$content'; | ||
| } | ||
| } | ||
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't mind me asking, as I'm working on reverting it now, is there a reason you have the
parsemethod pointing directly to theparseContentmethod, except for thedefaultDelimiterbit?Would you mind if I just put the contents of
parseContentdirectly into theparsemethod?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a functional programming style.
I would, unfortunately. I've just pushed a new release
1.1.0with some small changes. One of the changes is to rename theparseContentmethod toparser. I'd appreciate if you kept the current directory structure, with the two methods (parseandparseFile) exposed fromfront_matter.dartdelegating out to theparser()fromparser.dart.I recommend rebasing with the latest commits from master and starting from there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment, no. But this is subject to change.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MaterialTime the more I think about it, the less I'm convinced that this
FrontMatterDocumenttoStringmethod is possible. The method to generate the YAML string from aMapis outside the scope of this package, and frankly better suited to theyamlpackage.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.