Commit: Introduce commentChar and prettifyMessage in CommitOptions #745
Conversation
| /// <summary> | ||
| /// The starting line char used to strip lines if <see cref="PrettifyMessage"/> is true. Defaults to no stripping done. | ||
| /// </summary> | ||
| public char CommentChar { get; set; } |
There was a problem hiding this comment.
I could make it so if you set CommentChar, PrettifyMessage is set to true.
There was a problem hiding this comment.
How about making this a char??
There was a problem hiding this comment.
What would the benefit of having a nullable do here? We're using default(char) which is \0, same for the defaults in the functions like CreateCommit and prettify_message.
There was a problem hiding this comment.
There is nothing gained from this change. We still have to supply a character to prettify_message and it's the same: default(char) aka \0.
There was a problem hiding this comment.
There is nothing gained from this change. We still have to supply a character to prettify_message and it's the same: default(char) aka \0.
The defaults are supposed to be set in the constructor of the xxxOptions type. I'd rather not expose \0 as an already set property. Beside this, I see \0 as a libgit2 implementation detail and I'm sure it would make much sense to expose this at at the LibGit2Sharp API level.
|
This introduces changes in the test data since prettifying was defaulting to true in CreateCommit. |
|
/cc @nulltoken |
| @@ -22,5 +22,15 @@ public sealed class CommitOptions | |||
| /// True to allow creation of an empty <see cref="Commit"/>, false otherwise. | |||
| /// </summary> | |||
| public bool AllowEmptyCommit { get; set; } | |||
|
|
|||
| /// <summary> | |||
| /// True to prettify the <see cref="Commit"/> message, false otherwise. | |||
There was a problem hiding this comment.
True to prettify the message by stripping leading and trailing empty lines, trailing whitespace, and collapsing consecutive empty lines, false otherwise.
There was a problem hiding this comment.
Please use the constructor to set the default values. I'd suggest to go with PrettifyMessage set to true and CommentaryChar to null. Could you please also describe the default behavior as xml doc on the constructor (see MergeOptions for some inspiration 😉)?
There was a problem hiding this comment.
We should default prettify to true? Okay then, I guess that would revert the changes done to the mock object ids I changed.
|
Updated. |
| @@ -1070,7 +1070,7 @@ public static void git_merge_head_free(IntPtr handle) | |||
|
|
|||
| #region git_message_ | |||
|
|
|||
| public static string git_message_prettify(string message) | |||
| public static string git_message_prettify(string message, char commentChar = '\0') | |||
There was a problem hiding this comment.
I'd rather see this as a nullable char and pass \0 to libgit2 when it's indeed null.
There was a problem hiding this comment.
There was a problem hiding this comment.
Converting from char to sbyte when the char is 2-byte sized will throw an overflow exception. Wouldn't that suffice?
There was a problem hiding this comment.
I'm not sure an OverflowException would be that useful for the consumer API. The user wouldn't be able to easily understand what he should do to fix this. I think a special casing here with an explicit message may better communicate what's wrong here.
|
I think I fixed everything. I'm not explicitly setting CommentaryChar to null since that's redundant. default(Nullable) is null. |
| { | ||
| comment = Convert.ToSByte(commentChar.GetValueOrDefault()); | ||
| } | ||
| catch (OverflowException e) |
There was a problem hiding this comment.
I'm now generating an exception here. Please tell me if the text below should be changed.
There was a problem hiding this comment.
How about
Only single byte characters are allowed as commentary characters in a message (eg. '#').
|
Very nice work! Thanks! ✨✨✨ |
I'm still on a Mono which can't build so I'll be using travis to run the unit tests. :D