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

Make code more readable. #1

Merged
merged 1 commit into from Aug 10, 2012
Merged

Make code more readable. #1

merged 1 commit into from Aug 10, 2012

Conversation

ream88
Copy link
Contributor

@ream88 ream88 commented Aug 10, 2012

I know, it is a ridiculous change, but it makes the code much more readable. 😃

@MSch
Copy link
Owner

MSch commented Aug 10, 2012

I like it, but can you require 'active_support/core_ext/object/inclusion' on top?

MSch added a commit that referenced this pull request Aug 10, 2012
Make code more readable.
@MSch MSch merged commit 094789a into MSch:master Aug 10, 2012
@ream88
Copy link
Contributor Author

ream88 commented Aug 10, 2012

Done!

@MSch
Copy link
Owner

MSch commented Aug 10, 2012

Thanks @Haihappen!

@steveklabnik
Copy link

Ugh. I'm a big 👎 on this, not that it's my gem... we're actually not using in internally in Rails anymore, iirc... where was that ticket...

@ream88
Copy link
Contributor Author

ream88 commented Aug 10, 2012

@steveklabnik really? Why exactly? It's so much better to read imho.

@MSch
Copy link
Owner

MSch commented Aug 10, 2012

Well I agree that it's a lot easier to read, but if Rails stopped using it internally then that's a death sentence for in? in the long term (which in the rails world means 1 year) so I'd revert it.

In general I'm not that happy with many core exts in AS, but since ruby 2.0 will bring refinements (\o/) and since this gem will be used with Rails exclusively my thinking basically was to adhere to the Rails coding standards.

@steveklabnik
Copy link

I don't find it to be more readable. I recognize that this is taste.

It's also backwards from an OO perspective. You ask an array if something is in it, not something if it is in an array...

Anyway, yes, see this recent commit: rails/rails@447b6a4

@steveklabnik
Copy link

since ruby 2.0 will bring refinements (\o/)

:'( We apparently have very different tastes in Ruby.

@MSch
Copy link
Owner

MSch commented Aug 10, 2012

IMO Refinements are a superior solution to open classes, because they require opt-in. That way the're kinda like scala's implicits conversions/parameters, which are nice because they'd let me use - say - the old meta_search DSL that monkeypathes (implict conversion/refinement) Symbol next to whatever else that doesn't expect Symbol#greater_than

Anyway, that commit is a good reason to revert. Sorry @Haihappen.

@ream88
Copy link
Contributor Author

ream88 commented Aug 10, 2012

@MSch we should keep up with Rails Core team, so feel free to revert this commit!

@steveklabnik I should definitely read more Rails commits since Rails is a set of good practices. Will never use in? again 👅

@steveklabnik
Copy link

  • Refinements don't solve the problem. The whole point of AS is to do a global monkeypatch, it's not like AS will use refinements. And they'll want to support Ruby 1.9 for a while, so they wouldn't get used for a few years anyway.
  • Refinements are confusing. In what context do my strings have #foo again?
  • Refinements are slow. They reduce the speed of all code by about 10% (iirc, it used to be 30%), even code that doesn't use refinements.
  • Refinements make it hard for language implementors. The JRuby and Rubinius team have said that it's gonna make their lives really hard.
  • Refinements aren't classboxes. I haven't had my coffee, so I don't remember why I'm saying this, but it's another Ruby thing that's sorta-kinda like the CS concept, but a little bit off...

This thread isn't about refinements, though, so it's super OT, just sayin'.

@jandudulski
Copy link

it's also backwards from an OO perspective. You ask an array if something is in it, not something if it is in an array...

agree

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

4 participants