bigcartel / dugway

Easily build and test Big Cartel themes.
https://developers.bigcartel.com/api/themes
MIT License
149 stars 22 forks source link

preferred idiomatic ruby, multi-line block syntax #128

Closed jamelablack closed 9 years ago

biscuitvile commented 9 years ago

:+1:

Hi @jamelablack! Looks like CI is failing with some errors that I honestly would not expect from a change like this. I'll look into it for you because this should definitely be good to merge.

Also regarding your commit message, I like the style but personally I prefer to be a bit nitpicky. Obviously when looking at this repo's own history you can see some messages like this:

18fa7a9 Updated changelog
7e3a1df add changelog, bump version
0c9bbe4 Even moar fontz
db3aa88 Moar fontz
e7e94d4 add some docs re fonts

So obviously even in this project on its own there is not a solid history of setting consistent quality commit messages. I encourage you to check out the following articles when you have time:

http://rakeroutes.com/blog/deliberate-git/ http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html

In no way would I suggest this message should get in the way of merging a PR in this repo, but for example, using capitalization as Tim Pope suggests, and using present tense as Stephen Ball suggests makes for a better commit message: "Prefer idiomatic ruby, multi-line block syntax"

This is a good old fashioned nitpick, in the positive spirit of Katrina Owen's http://exercism.io nitpicks feature :)

jamelablack commented 9 years ago

Thanks @biscuitvile for the feedback! I'll take a look at the resources you suggested. :)

jamelablack commented 9 years ago

Updated commit message. Using feature branches instead of master.