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

removing duplicate code in railties/lib/rails/generators/resource_helper... #13771

Closed
wants to merge 1 commit into from

Conversation

@anilmaurya
Copy link
Contributor

anilmaurya commented Jan 20, 2014

...s.rb

@vipulnsward
Copy link
Member

vipulnsward commented Jan 20, 2014

lgtm.

@chancancode
Copy link
Member

chancancode commented Jan 20, 2014

Thanks for your contribution! However, this is a cosmetic change, and as noted in the the contributing to rails guide, we have a policy of not accepting these kind of changes.

I know this might be thinking, "sure, it might not be adding much value, but I already wrote the code, so the cost is already paid – by me – so why not just merge it"? The reason is that there are a lot of hidden cost in addition to writing the code itself.

  • Someone need to spend the time to review the patch. However trivial the changes might seem, there might be some subtle reasons the original code are written this way and any tiny changes have the possibility of altering behaviour and introducing bugs. (For example, in this case, do you know if self.name= and assign_names doesn't depend on controller_name being set? I don't – so I'd have to look it up and make sure. Also, there is a travis failure associated with this PR, it's probably a random failure, but I'd have to investigate to be sure). All of these work takes away time and energy that could be spent on actual features and bug fixes.
  • It creates noise. There are currently 1,362 people watching this repo at the time of writing – these people will get an email from github everytime someone opens a new issue, comment on a ticket, etc. They do this (probably) because they want to watch out for PRs and issues that they care about, and these PRs will further lower the signal-to-noise ratio in these notification emails.
  • It pollutes the git history. When someone need to investigate a bug and git blame these lines in the future, they'll hit this "refactor" commit which is not very useful.
  • It makes backporting bug fixes harder.

Theses are just some examples of the hidden costs that are not so apparent from the surface.

It's awesome that you want to contribute to Rails, please keep the PRs coming! All we ask is that you refrain from sending these types of changes in the future (and read the contribution guide :). I hope you'll understand!

P.S. I'm not picking on you – it's just that we seem to be getting more PRs in the cosmetic changes category, and so I wanted to take this chance to explain our position and have something I can link to in the future.

❤️ 💚 💙 💛 💜

@anilmaurya
Copy link
Contributor Author

anilmaurya commented Jan 20, 2014

@chancancode I understand your point, but i am surprised to see that Rails community do not promote improving code quality.

I am using code climate for past few months , and am confident that my commit will surely decrease method complexity, do not mind if you still think its a cosmetic change.

@rafaelfranca
Copy link
Member

rafaelfranca commented Jan 20, 2014

Looks like it is not a cosmetic change neither duplicated code. Tests are broken with this change.

@bogdanvlviv bogdanvlviv mentioned this pull request Aug 8, 2016
@mtsmfm mtsmfm mentioned this pull request Feb 28, 2017
@@ -16,12 +16,10 @@ def self.included(base) #:nodoc:
def initialize(*args) #:nodoc:
super
if options[:model_name]
controller_name = name
self.name = options[:model_name]

This comment has been minimized.

Copy link
@RKushnir

RKushnir Apr 29, 2020

Contributor

Name gets changed here, so we cannot move controller_name assignment below this line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.