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

Change unless + else into if + else #6182

Merged
merged 1 commit into from May 7, 2012

Conversation

shaliko
Copy link
Contributor

@shaliko shaliko commented May 6, 2012

@gazay
Copy link
Contributor

gazay commented May 6, 2012

I like unless-else in some cases and I don't think that it is necessary to change it here

@rafaelfranca
Copy link
Member

We are avoiding this kind of changes in the codebase because they make hander to dig in the git history to track bugs. But they are welcome when you are adding features or solving bugs.

Thanks.

@drogus
Copy link
Member

drogus commented May 7, 2012

@rafaelfranca I think that code changes to make it easier to understand were usually accepted

@rafaelfranca
Copy link
Member

Well, I saw some pull request being closed with the same kind of changes. Some examples 6100, 6000.

And, I don't think that this change make easier to understand, for me is the same thing.

But you can accept it if you want.

@drogus
Copy link
Member

drogus commented May 7, 2012

@rafaelfranca thanks for references, haven't noticed that before. I agree with that, just was not sure if that's what core team basically recommends

@carlosantoniodasilva
Copy link
Member

I have to agree about changing all instances of X for Y, it can really makes things harder later on. But I have to say that in my opinion there wouldn't be any harm in accepting minor readability improvements. We do have limited time, that doesn't mean we can't help someone that tries to contribute, being it with bugs or improvements. Just my 2 cents ;)

@josevalim josevalim reopened this May 7, 2012
josevalim added a commit that referenced this pull request May 7, 2012
@josevalim josevalim merged commit c35df12 into rails:master May 7, 2012
@josevalim
Copy link
Contributor

Merged. In any case, the issue with merging these pull requests is that it sends a message to everyone else that we are ok with such changes while I would prefer not to merge them in the first place (different committers may have different opinions).

Again, we have little time and these things do accumulate.

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

6 participants