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

improve redability #6100

Closed
wants to merge 1 commit into from
Closed

improve redability #6100

wants to merge 1 commit into from

Conversation

shime
Copy link
Contributor

@shime shime commented May 1, 2012

Hi guys,

I did some refactoring of single-line class definitions. Class.new seems more readable than using semicolon, don't you think?

Looking forward for your feedback.

use Class.new for single line class definitions
remove some whitespace
@@ -1,9 +1,7 @@
module ActionController
class ActionControllerError < StandardError #:nodoc:
end
ActionControllerError = Class.new StandardError
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this 👎

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gazay why?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just don't like this syntax, as I think < more clear for understanding that it is inheritance.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

< is syntax sugar for Class.new :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, so how I said - I don't like Class.new syntax if we can use more readable <:)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand. I just said that < was created for readability IMO :)

@josevalim
Copy link
Contributor

In general we avoid doing changes throughout Rails source code that are meant just to improve readability. If you are refactoring a part of the code and rewrite it, it is fine, but we ask to avoid sending changes that replace all occurrences of X by Y because it may read better.

@josevalim josevalim closed this May 1, 2012
@shime
Copy link
Contributor Author

shime commented May 1, 2012

Cool, sorry about this then.

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