search

kschiess / parslet

A small PEG based parser library. See the Hacking page in the Wiki as well.
kschiess.github.com/parslet
MIT License
805 stars 95 forks source link

use basic object instead of blankslate #134

Closed rubydesign closed 9 years ago

rubydesign commented 9 years ago

this time with good old text mate: no whitespace changes

rubydesign commented 9 years ago

Well, nice to hear the 20% of information in there. The 80% of "i'm tired of this" was a bit much though. (You kid yourself with it's not personal: which you will know when someone says it to you)

You finish on a saving grace. But you fail to actually say what you need for this to pass. Maybe if you write some tests that i could pass. Or tell in concrete terms what that "quality" is. Do you need a responds_to? or what do you need. You did understand that the methods (instance_eval , etc..) do work, yes?

kschiess commented 9 years ago

Please make a proposal as to what you would merge in my place.

:v:

rubydesign commented 9 years ago

I would merge my pr,also as you. Not only would i understand that by asking for the rework i had sort of pre-approved the merge, but also show appreciation for the work.

If i didn't want to merge a pr that passes all test, i would write a test (or tests) that pass under master but fail on the pr and ask for a fix.

kschiess commented 9 years ago

I've provided a few such failing specifications. I'll have time to provide more if needed; let's make this good.

rubydesign commented 9 years ago

well, great, and yes, let's !!

I think this can be solved with singleton_method_added, i'll get on it

rubydesign commented 9 years ago

ok, the tests pass :-)

I tried to rebase so you'd have it easier. But since i've never done that, i may have messed up a little :-(

Have a look and tell me what you think

kschiess commented 9 years ago

The misunderstanding here is about PR acceptance criteria: In parslet, your code needs to a) have green specs (of course) but also b) be somewhat elegant. Reimplementing #methods and #respond_to? in BasicObject because they are missing is certainly not elegant; and I don't want to write the tests to prove your approach here flawed; these methods mean something to Rubyists and should mean the same during context instance_evals. Please look up the details in the documentation.

BasicObject is a bad base for reconstructing BlankSlate; I've said it before and know all about it. Anyone claiming the opposite should do the research.

I guess by now replacing BlankSlate with Object is far more promising. Maybe look into that? What are the downsides? Aka: What needs to be removed from Object to allow painless access to captures during an Context-instance_eval?

rubydesign commented 9 years ago

You know, i'm good. In other words your terms of 'co-operation' are not for me.