Skip to content

Conversation

@spoonscen
Copy link

Was reading through the README and spotted the function name in "Don't add unneeded context" breaks the rule "Function names should say what they do". Want to throw a quick PR up for you!

Thanks!

@ryanmcdermott
Copy link
Owner

Oo thanks for looking at this, it seems we could actually modify this function to be:

function paintCar(car, color) { car.color = color; }

Would you have the chance to add a second commit to your PR to make the function look like that?

@spoonscen
Copy link
Author

Definitely! I wanted to just make the smallest change possible, but I agree that a function that only paints a car red quite specific!

Copy link

@felquis felquis left a comment

Choose a reason for hiding this comment

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

Okay guys, so this PR is really good and can be merged without thinking twice!

@ryanmcdermott
Copy link
Owner

Fixed in 3ff9eba

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants