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

Even more split outs from #339 #345

Merged
merged 4 commits into from
Jun 13, 2015
Merged

Even more split outs from #339 #345

merged 4 commits into from
Jun 13, 2015

Conversation

arlolra
Copy link
Contributor

@arlolra arlolra commented Jun 8, 2015

No description provided.

@arlolra
Copy link
Contributor Author

arlolra commented Jun 8, 2015

Ok, that should be it for #323

@dmajda
Copy link
Contributor

dmajda commented Jun 11, 2015

Can you please split any changes not directly related to enabling strict mode into separate PR(s)? (Where directly related = without them code doesn’t work properly or JSHint produces errors.)

@arlolra
Copy link
Contributor Author

arlolra commented Jun 11, 2015

Sure, coming right up.

@arlolra
Copy link
Contributor Author

arlolra commented Jun 11, 2015

Ok, updated the branch.

@dmajda
Copy link
Contributor

dmajda commented Jun 12, 2015

Thanks for the work, I commented couple of minor issues.

@arlolra
Copy link
Contributor Author

arlolra commented Jun 12, 2015

Thanks for the review. I'll have these issues sorted shortly.

Can you see my comment above about the leading ;.

@dmajda
Copy link
Contributor

dmajda commented Jun 12, 2015

FYI I just realized there is an older PR (#334) implementing essentially the same changes that were part of this PR before I asked for focusing on #323. I’ll work with the author to get them in because he was first but if that gets stale, a PR from you is still welcome.

@arlolra
Copy link
Contributor Author

arlolra commented Jun 13, 2015

Should be better now.

I left in the node directive in jshint and explicitly disabled it for benchmark/index.js. Otherwise, the majority of the files complain. Happy to change that if you prefer it another way.

@dmajda
Copy link
Contributor

dmajda commented Jun 13, 2015

Should be better now.

Looks great, thanks! Merging in a second.

I left in the node directive in jshint and explicitly disabled it for benchmark/index.js. Otherwise, the majority of the files complain. Happy to change that if you prefer it another way.

Makes sense to me.

dmajda added a commit that referenced this pull request Jun 13, 2015
@dmajda dmajda merged commit cb640cd into pegjs:master Jun 13, 2015
@dmajda
Copy link
Contributor

dmajda commented Jun 13, 2015

@arlolra As a side note, I’m on vacation starting tomorrow, so if you come up with more PRs I may react slowly or not at all for the next 2-3 weeks.

@arlolra
Copy link
Contributor Author

arlolra commented Jun 13, 2015

Thanks, and have a nice vacation! I'll probably work on the interruptible parser plugin in the meantime.

@arlolra arlolra deleted the split4 branch June 13, 2015 05:46
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