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

Wrap commit messages at 72 chars #133

Closed
wants to merge 1 commit into from
Closed

Conversation

rakoo
Copy link

@rakoo rakoo commented Jun 24, 2012

It is a fairly accepted guideline that commit messages should wrap at 72 chars.

Possible improvements :

  • Same kind of thing for the commit title, which shouldn't exceed 50-60 characters.
  • Wrap at 72 chars AND not at widget width.
  • Get the '72' magic number out of a config file.

It is a fairly accepted guideline that commit messages should wrap at 72 chars.
@ugtar
Copy link
Member

ugtar commented Jun 25, 2012

Not sure how I feel about this...

I agree that 72 chars is the accepted standard, and I keep to it
myself, but is it necessary for cola to enforce it? Cola already
politely suggests that you wrap at 72 via the helpful color coded
column counter.

What if others want to have their own standards for their own
projects? In addition, there are exceptions to the 72 char rule (see
here: torvalds/linux#17 (comment)).

To me a good way to do this, if it's desired would be to add a toggle
checkbox next to the column counter that offers to automatically wrap
at 72, but which could easily be switched off temporarily or kept off
if the user wants to manage her own linebreaks.

I don't think cola should force without giving you the option to opt-out.

On Sun, Jun 24, 2012 at 11:43 PM, Matthieu Rakotojaona
reply@reply.github.com
wrote:

It is a fairly accepted guideline that commit messages should wrap at 72 chars.

Possible improvements :

  • Same kind of thing for the commit title, which shouldn't exceed 50-60 characters.
  • Wrap at 72 chars AND not at widget width.
  • Get the '72' magic number out of a config file.

You can merge this Pull Request by running:

 git pull https://github.com/rakoo/git-cola wrap-commit

Or you can view, comment on it, or merge it online at:

 #133

-- Commit Summary --

  • Wrap commit messages at 72 chars

-- File Changes --

M cola/widgets/commitmsg.py (3)

-- Patch Links --

 https://github.com/git-cola/git-cola/pull/133.patch
 https://github.com/git-cola/git-cola/pull/133.diff


Reply to this email directly or view it on GitHub:
#133

   Uri

Please consider the environment before printing this message.
http://wwf.panda.org/savepaper/

@ugtar
Copy link
Member

ugtar commented Jun 25, 2012

Btw, I know the Linus comment I linked to is actually arguing in favor
of auto-linebreaks, but he's arguing for it in the github interface
which suffers from no way to tell when you are at 72. Cola doesn't
suffer from this problem and provides a very unobtrusive way to keep
you informed and let you know when you're doing something "bad".

On Mon, Jun 25, 2012 at 9:30 AM, Uri Okrent uokrent@gmail.com wrote:

Not sure how I feel about this...

I agree that 72 chars is the accepted standard, and I keep to it
myself, but is it necessary for cola to enforce it?  Cola already
politely suggests that you wrap at 72 via the helpful color coded
column counter.

What if others want to have their own standards for their own
projects?  In addition, there are exceptions to the 72 char rule (see
here: torvalds/linux#17 (comment)).

To me a good way to do this, if it's desired would be to add a toggle
checkbox next to the column counter that offers to automatically wrap
at 72, but which could easily be switched off temporarily or kept off
if the user wants to manage her own linebreaks.

I don't think cola should force without giving you the option to opt-out.

On Sun, Jun 24, 2012 at 11:43 PM, Matthieu Rakotojaona
reply@reply.github.com
wrote:

It is a fairly accepted guideline that commit messages should wrap at 72 chars.

Possible improvements :

  • Same kind of thing for the commit title, which shouldn't exceed 50-60 characters.
  • Wrap at 72 chars AND not at widget width.
  • Get the '72' magic number out of a config file.

You can merge this Pull Request by running:

 git pull https://github.com/rakoo/git-cola wrap-commit

Or you can view, comment on it, or merge it online at:

 #133

-- Commit Summary --

  • Wrap commit messages at 72 chars

-- File Changes --

M cola/widgets/commitmsg.py (3)

-- Patch Links --

 https://github.com/git-cola/git-cola/pull/133.patch
 https://github.com/git-cola/git-cola/pull/133.diff


Reply to this email directly or view it on GitHub:
#133

   Uri

Please consider the environment before printing this message.
http://wwf.panda.org/savepaper/

   Uri

Please consider the environment before printing this message.
http://wwf.panda.org/savepaper/

@rakoo
Copy link
Author

rakoo commented Jun 25, 2012

I totally agree with you. In fact this commit is more like a Proof-of-Concept to see how it could be done. Hacking on python was so easy that I could be done with it in minutes.

What I had in mind was something like this :

  • Wrap at 72 chars by default;
  • If the user enters a 'colwidthwrap' param with 0 as a value, it means he wants no wrap;
  • If he enters the same param with a value > 0, it means he wants this exact value

This will have the advantage to auto-format the commit messages for people who don't know about a very spreaded guideline.

I don't think a checkbox would be a so good idea in the long run : it eats up some screen estate for a setting you will use once at the beginning of your use of git-cola, and never then. If you are curious about it, there would be the doc, as always.

Cola doesn't suffer from this problem and provides a very unobtrusive way to keep you informed and let you know when you're doing something "bad"

And this is one of the best things I have ever seen in a GUI that has some text-editing capabilities but is not a full-blown text editor. Thank you for that !

@ugtar
Copy link
Member

ugtar commented Jun 25, 2012

Hi

On Mon, Jun 25, 2012 at 10:00 AM, Matthieu Rakotojaona
reply@reply.github.com
wrote:

I totally agree with you. In fact this commit is more like a Proof-of-Concept to see how it could be done. Hacking on python was so easy that I could be done with it in minutes.

What I had in mind was something like this :

  • Wrap at 72 chars by default;
  • If the user enters a 'colwidthwrap' param with 0 as a value, it means  he wants no wrap;
  • If he enters the same param with a value > 0, it means he wants this exact value

Since 72 is the standard, having it be configurable might be overkill.
We already have the color coding hard coded to 72, so it wouldn't be
too bad to make the wrapping do that. But, having a param somewhere
couldn't hurt.

This will have the advantage to auto-format the commit messages for people who don't know about a very spreaded guideline.

I agree, having it on should be the default.

I don't think a checkbox would be a so good idea in the long run : it eats up some screen estate for a setting you will use once at the beginning of your use of git-cola, and never then. If you are curious about it, there would be the doc, as always.

I disagree with this. Having a quick way to "opt-out" right as you're
entering text is nice because it's easy to do things like add log
messages or compiler messages that shouldn't be auto-wrapped. In
addition, there's plenty of room in the header bar and we could make
something pretty tiny that works fine. I made a mock-up. The label
says "Auto-wrap", but it could probably even just say "wrap".

wrap option mockup

   Uri

Please consider the environment before printing this message.
http://wwf.panda.org/savepaper/

@rakoo
Copy link
Author

rakoo commented Jun 25, 2012

I'll close the pull request, because it will never be merged as-is. Discussion continues in the mailing list :
https://groups.google.com/forum/?#!topic/git-cola/bGNw6OqTcyc

@rakoo rakoo closed this Jun 25, 2012
davvid added a commit that referenced this pull request Jul 18, 2012
Teach the commit message editor to wrap long lines.  We reuse the vim semantics,
namely `cola.linebreak` and `cola.textwidth` are the configuration knobs.

This feature is enabled by default but it can be toggled on a one-off basis
by using the commit message editor's sprocket / settings menu.

Related-to: #133
Suggested-by: Matthieu Rakotojaona <matthieu.rakotojaona@gmail.com>
Helped-by: Uri Okrent <uokrent@gmail.com>
Signed-off-by: David Aguilar <davvid@gmail.com>
@davvid
Copy link
Member

davvid commented Jul 20, 2012

I'll probably be merging 6263957 sometime next week.

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

3 participants