Fixes for issues 28 - 36 #38
Conversation
at bottom of message body.
|
@bshannon Thank you very much for sharing your patches in such a clean way. I need some time to finish evaluating them. Most of them look good. The only one I'm not sure is the iconvert one. I'm asking for a second opinion on that one from an i18n expert to see if your solution is the best generic one. |
|
Thanks. Let me know if you have any questions. I'm not an i18n expert either, and I suspect there are still problems in that area. The entire i18n support is very ad hoc and full of assumptions about how charsets work. The iso-2022-jp support is done as a special case, instead of handling different charsets in a more general way. |
|
@jkbzh Just checking in to see if anyone has had a chance to look at these changes... |
|
Looks like no one has any time to deal with this... :-( |
|
Any progress on this? |
|
@jkbzh Just checking in again to see if anyone has had time to look at my fixes... |
| @@ -833,6 +834,14 @@ <h3><a name="msg_head">Message page headers/footers</a></h3> | |||
| <br><i>show_headers = From,Subject,Date,Message-ID</i> (disabled by default) | |||
| </dd> | |||
|
|
|||
| <a name="show_received_date"></a> | |||
| <dt><strong>show_received_date = [ 0 | 1 ]</strong></dt><dd> | |||
There was a problem hiding this comment.
Please add (enabled by default) comment.
There was a problem hiding this comment.
I updated the example and added the comment, as is done for other options that are enabled by default.
src/string.c
Outdated
| @@ -1485,7 +1485,7 @@ char *parseemail(char *input, /* string to parse */ | |||
| at="@"; | |||
|
|
|||
| if (strchr(input, '@') == NULL && strstr(input, "@") == NULL) | |||
| return input; // nothing to do here | |||
| return strsav(input); // nothing to do here | |||
There was a problem hiding this comment.
This patch is taken out of context so it's hard to tell to which other part of the code it is related to Rather than sending the correction, could you resubmit this patch with its other associated code changes?. (only for the issue this one is solving).
| if (!setlocale(LC_ALL, locale_code)) { | ||
| if (!setlocale(LC_ALL, locale_code)) { | ||
| char *locale_code_utf8 = utf8locale(locale_code); | ||
| if (!setlocale(LC_ALL, locale_code_utf8)) { | ||
| snprintf(errmsg, sizeof(errmsg), "WARNING: locale \"%s\", not supported.\n", locale_code); | ||
| fprintf(stderr, "%s", errmsg);/* AUDIT biege: avoid format-bug warning */ |
There was a problem hiding this comment.
This patch look s ok. Could you explain what is "AUDIT biege"?
There was a problem hiding this comment.
No idea. That's not code that I added or touched.
|
Hello @bshannon,
Finally I have work time assigned for hypermail and I started
reviewing your changes. Once again thank you for your contribution
and for being so patient.
I have a problem with your contributions: they solve many issues
and it's a big and monolythic PR. It makes it so much harder to
review and apply changes issue per issue. It's beter to send
a MR per issue.
For example in your MR, you have two different commits for show_received
data, one for the code, and another for the documentation. If it
were a single PR, I could have already merged it. Instead, I've to wait
until I review all the other changes or cherry pick them one by one.
Another example, your last MR message titled "Oops, forgot to ..."
is completely out of context. I don't know to which other change
it relates. If it were in a single PR, it would show right away.
As a best practice, it's better to keep the changes small in each PR,
to solve an issue, then submit another PR for another issue.
https://readwrite.com/2014/07/02/github-pull-request-etiquette/
Could you resubmit them as different PR, grouped one per bug / enhancement
(e.g., put all changes related to show_received together in the same PR).
I'll then be able to integrate them one by one much faster and keep the
discussion / research for the difficult ones separated.
Thank you,
|
|
Welcome back! :-) What's an "MR"? These commits were done incrementally as I found and fixed bugs. That's why it Do I need to create a branch for each issue? And then create another branch that merges If that's more or less correct, let me see if I can redo this as a set of branches and pull requests. Also, I got a bunch of github notifications about comments you added, but I'm not seeing |
|
Ok, I think I've figured this out. New pull requests coming... |
|
New pull requests submitted, closing this one. |
@bshannon, can you paste one of those notifications? |
|
Here's a notification I got that I can't see in the "Files changed" tab: @jkbzh commented on this pull request. In docs/hmrc.4:
From line 760, you mean to say this option is disabled by default? It should be the case as it is a new feature that may change the output. |
|
Hum, I can't find that either, @bshannon… Didn't that message include a direct link to the specific commit, review, comment… something that might help find it? |
|
The text "docs/hmrc.4" is linked to #38 (comment). The "view it on GitHub" link is to #38 (review) Neither of them take me directly to the comment. And if I switch to the "Files changed" tab, the first file is docs/hmrc.4. It shows the changes, but it doesn't include a "Show comments" check box. |
I needed all these fixes to get hypermail to handle some mail archives I'm dealing with.
These changes have only been tested with my specific use of hypermail, so let me know
if you see any problems.