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

Replace BlankSlate with BasicObject #154

Merged
merged 1 commit into from
May 18, 2016
Merged

Replace BlankSlate with BasicObject #154

merged 1 commit into from
May 18, 2016

Conversation

flash-gordon
Copy link
Contributor

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 (txus/kleisli#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
Copy link
Contributor Author

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
Copy link
Owner

kschiess commented May 9, 2016

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
Copy link
Contributor Author

@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
Copy link
Owner

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:

  • a clean context object
  • warnings for captures that can't be exposed as methods locally for some reasons (what are those? does this situation exist at all?) - This is to not violate expectations.

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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.

@kschiess kschiess merged commit dcc36a8 into kschiess:master May 18, 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.

None yet

2 participants