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

Scalar parser #37

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Scalar parser #37

wants to merge 3 commits into from

Conversation

DeltaEvo
Copy link

Fix #35

@DeltaEvo
Copy link
Author

What do you think about this pr @chyh1990 ?

@chyh1990
Copy link
Owner

chyh1990 commented Nov 4, 2016

The API enhancement looks good for me. One little problem, maybe we should move dump_node into a helper in examples, instead of just marking it as pub.

Copy link

@flyx flyx left a comment

Choose a reason for hiding this comment

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

Since @chyh1990 kindly mentioned me, I provided some comments. Keep in mind that I am neither familiar with Rust nor with this YAML implementation and my assumptions may be wrong.

impl<'a> yaml::YamlScalarParser for IncludeParser<'a> {
fn parse_scalar(&self, tag: &scanner::TokenType, value: &str) -> Option<yaml::Yaml> {
if let scanner::TokenType::Tag(ref handle, ref suffix) = *tag {
if *handle == "!" && *suffix == "include" {
Copy link

Choose a reason for hiding this comment

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

This line implies that the model for tag parsing implemented here differs from the YAML spec. A tag handle is nothing more but a shorthand to a URI prefix. The primary tag handle ! defaults to the URI prefix ! but may be redefined by a %TAG directive, so if a tag !include occurs somewhere in the source, it may resolve to the URI !include, but also to my.fancy.uri.prefix:include. Example:

%YAML 1.2
%TAG ! my.fancy.uri.prefix:
---
!include foo

For a parser to be spec-compliant, one needs to resolve all tags to URIs and then compare those URIs against whatever is expected.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks I don't know this feature of Yaml 1.2 spec

@DeltaEvo
Copy link
Author

DeltaEvo commented Nov 5, 2016

@chyh1990 How do you want to move dump_node as helper ?

@DeltaEvo
Copy link
Author

DeltaEvo commented Nov 16, 2016

@DeltaEvo
Copy link
Author

DeltaEvo commented Dec 7, 2016

@chyh1990 any news about the build system ?

@chyh1990
Copy link
Owner

chyh1990 commented Dec 8, 2016

@DeltaEvo you can try rebasing your pr, master passes CI now.

@DeltaEvo
Copy link
Author

DeltaEvo commented Dec 8, 2016

@chyh1990 I think I'm unlucky

@chyh1990
Copy link
Owner

chyh1990 commented Dec 9, 2016

@DeltaEvo Seems clippy breaks on newest nightly...

https://github.com/Manishearth/rust-clippy/issues/1371

@DeltaEvo DeltaEvo force-pushed the scalar_parser branch 5 times, most recently from 807435a to 5f2d7e4 Compare December 20, 2016 16:52
@DeltaEvo
Copy link
Author

@chyh1990 The issue on clippy is fixed and I rebased my Pull Request

@DeltaEvo
Copy link
Author

@chyh1990 or @dtolnay, what is missing in this PR from being merged ?
Thank you

@zuowenjian
Copy link

merge failed ?

@gyscos gyscos mentioned this pull request Aug 6, 2019
@gyscos
Copy link

gyscos commented Aug 6, 2019

I rebased this branch in a separate PR: #135

@jaesharp
Copy link

jaesharp commented Oct 1, 2019

Given that #135 replaces this PR, should this PR be closed?

@dtolnay
Copy link
Collaborator

dtolnay commented Aug 1, 2022

@dtolnay, what is missing in this PR from being merged ?

I am not involved in this crate, since it is no longer used by serde_yaml.

@davvid
Copy link

davvid commented Jan 29, 2024

FWIW I've merged #135 (a rebased version of #37 (this PR)) into my fork: https://github.com/davvid/yaml-rust

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

Successfully merging this pull request may close these issues.

Adding Support for Tag Directives
8 participants