kschiess / parslet

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

Replace BlankSlate with BasicObject #154

Closed flash-gordon closed 8 years ago

flash-gordon commented 8 years ago

Hello,

I faced with a weird error when I tried to use parslet with this gem https://github.com/txus/kleisli (it is dry-types dependency). I did some research and found that the issue is already resolved in master branch (https://github.com/txus/kleisli/pull/17).

Anyway, I decided that blankslate is not required anymore and replaced it with equivalent BasicObject ancestor.

It would not be necessary but I think that using #method_added is really dark magic and we should avoid it as much as we can. Moreover blankslate adds hooks to Kernel, Object and Module classes (see https://github.com/masover/blankslate/blob/master/lib/blankslate.rb#L62) and it's really really bad.

I tested my changes on ruby 2.0 and higher and all specs passed. I failed to compile 1.9.3 on my machine but I expect that tests would fail because there were some changes about binding methods in ruby 2.0. In that case you could drop 1.9 support but this decision is up to you. You are free to merge PR at anytime you want (=

flash-gordon commented 8 years ago

2.1 specs actually pass: https://travis-ci.org/flash-gordon/parslet 1.9 fails because flexmock dependency now requires ruby 2.0 or higher. Still do not know if it is able to work

kschiess commented 8 years ago

Thank you for caring!

Please have a look at

https://github.com/kschiess/parslet/pulls?q=is%3Apr+blankslate+is%3Aclosed

I start to wear out on the topic - maybe I'll merge this just to not have to explain it anymore. Really what I would prefer: Base the object context on Object simply. With some warnings for when variables are shadowed by methods. Feel like jumping to our rescue here?

flash-gordon commented 8 years ago

@kschiess thanks for answering!

I was really concerned about preserving the interface, believe me. Blankslate does really nasty things by adding method_added hooks to Object and Kernel and they are the reason why I send the PR: I ran into stack overflow exception when I tried to use parslet alongside kleisli gem. It is extremely unwelcome error so I tried to fix the issue by removing blankslate from both gems in order to avoid any possibility to see the exception in the future. Thankfully kleisli already had the patch (which is not released up to date though).

In the PR I added to BasicObject just the same interface as Blankslate provides, you can try to break it and I would be very happy to fix any cases you find. Not to mention there is no monkey patching in any form so it absolutely safe for the runtime.

And of course I'm OK with replacing BasicObject with Object but the decision is up to you. And I can do it for you, just ask.

kschiess commented 8 years ago

As I've stated before, I fail to see where BasicObject and blankslate are similar, except perhaps in goal formulation. But that is really not the issue - what I think parslet would need is the following:

I think this could easily be based on Object. If you want to go down that route, I'll gladly read your code and eventually merge.

flash-gordon commented 8 years ago

@kschiess I updated the PR! Technically method calling is just passing an arbitrary string to an object and you can call any method using Kernel#send. So I don't think there are any special reasons about method names in general however not all methods can be called by name via ordinary ruby syntax. Do you think we should print a warning in such cases? Personally I don't know.

flash-gordon commented 8 years ago

Ah, and what do you about spaces at line endings? It seems that most of contributors (including me) have a trimming setting turned on by default? Would it be easier just to trim all hanging spaces in the project in a single commit so that you don't need to ask every one to not include trimmed lines to a commit? :) I can handle it as well ;) Or maybe it's better to use rubocop for styling the code.