Skip to content

Parsing fixes #1

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

Merged
merged 3 commits into from
Dec 23, 2016
Merged

Parsing fixes #1

merged 3 commits into from
Dec 23, 2016

Conversation

jugglinmike
Copy link
Contributor

Extend YAML parser

Some recent changes in Test262 rely on previously-unused aspects of the YAML
grammar. Update the simplified YAML parser contained in this project to support
them.

@leobalter @littledan @tschneidereit Any of you folks mind reviewing this and
merging it if it looks okay?

@littledan
Copy link
Contributor

Thanks for following up with this. So that I can test your code, which is the test262 test that depends on this change? Is it committed in master?

@jugglinmike
Copy link
Contributor Author

Actually, it looks like these changes aren't necessary in isolation. For full
context: I made them in support of the "negative
reform"
patch that recently landed
in Test262. Although this change set does not introduce support for that new
format, it's a prerequisite. I was hoping that the review process would be
more streamlined if I submitted the work in two phases given that these changes
concern valid aspects of the YAML grammar. But since there's not actually a use
case for them in isolation, the separation probably doesn't make sense.

Now that the previously-mentioned Test262 patch has landed, that final commit
can be landed here, as well. I've introduced it to this branch.

It occurs to me that this patch removes support for the prior error format
outright. As far as I know, V8 is the only consumer of this project, so that be
acceptable.

@littledan
Copy link
Contributor

For V8's part, I only care about the ToT test262-harness-py supporting the ToT test262; as long as that's preserved, I'm happy.

@tschneidereit are you consuming this project?

@littledan
Copy link
Contributor

The changes all look look good to me by visual inspection. Note that we don't use test262.py in the V8 test setup. What do you think the chances are of landing this patchset? If it could land, it would help us advance to the new version of test262 requiring the monkeyaml changes.

@littledan
Copy link
Contributor

I have verified that this yaml parser works for the test262 tests for me within V8's setup, and that the tests won't work without it :). So there's some more evidence for landing it.

@jugglinmike
Copy link
Contributor Author

Hey Dan,

I stand by this patch, but I'm loathe to merge my own work on any project. I
consider your review to be sufficient verification, but I'd still rather you
did the honors. When we created the "test262-utils" GitHub organization, I
invited both you and @tschneidereit to become members. The GitHub UI lists
those invitations as "pending," so I suspect if you hunt around your account
panel (or your e-mail inbox), you will find a message about the invitation and
a means to accept. From there, it's just a button push away!

@littledan littledan merged commit 0f2acdd into master Dec 23, 2016
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.

2 participants