1. Forum
  2. FidoNet
  3. PRISM

  • feedback on small mm patch

    From Andrew Alt@1:261/38 to All on Thu Aug 8 14:24:54 2019
    Was wondering if anyone could give me some additional feedback on this small patch
    I submitted for MultiMail?

    https://github.com/wmcbrine/MultiMail/pull/20

    That shows the feedback I got so far, clicking the "files changed" tab shows the
    actual patch.

    Thanks...

    --
    -Andy


    ... Got my tie caught in the fax... Suddenly I was in L.A.
    -+- MultiMail/Linux v0.52

    --- BBBS/Li6 v4.10 Toy-4
    * Origin: Prism bbs (1:261/38)
  • From Alan Ianson@1:153/757 to Andrew Alt on Thu Aug 8 12:53:44 2019
    Andrew Alt wrote to All <=-

    Was wondering if anyone could give me some additional feedback on this small patch I submitted for MultiMail?

    https://github.com/wmcbrine/MultiMail/pull/20

    That shows the feedback I got so far, clicking the "files changed" tab shows the actual patch.

    What's the patch do? I'll have look soonish.. :)

    Ttyl :-),
    Al

    ... I couldn't repair the brakes.. So I made your horn louder!
    -+- MultiMail/Linux v0.52

    --- BBBS/Li6 v4.10 Toy-4
    * Origin: The Rusty MailBox - Penticton, BC Canada (1:153/757)
  • From Andrew Alt@1:261/38 to Alan Ianson on Fri Aug 9 15:01:24 2019
    Alan Ianson wrote to Andrew Alt <=-

    Andrew Alt wrote to All <=-

    Was wondering if anyone could give me some additional feedback on this small patch I submitted for MultiMail?

    https://github.com/wmcbrine/MultiMail/pull/20

    That shows the feedback I got so far, clicking the "files changed" tab shows the actual patch.

    What's the patch do? I'll have look soonish.. :)

    Not much of anything really xD It's pretty trivial.. but thought it was worth doing. I narrowed the scope of newtag and put the fopen statement next to the declaration of the file descriptor, and made a slight change to the condition (which probably could just be "flag = (fd);" but the PR was closed so... guess won't make any more changes to it...


    -+-
    interfac/tagline.cc | 6 ++----
    1 file changed, 2 insertions(+), 4 deletions(-)

    diff --git a/interfac/tagline.cc b/interfac/tagline.cc index 3020655..a7e836d 100644
    -+- a/interfac/tagline.cc
    +++ b/interfac/tagline.cc
    @@ -259,13 +259,11 @@ void TaglineWindow::kill()
    bool TaglineWindow::ReadFile()
    {
    FILE *fd;
    - char newtag[TAGLINE_LENGTH + 1];
    - bool flag;
    -
    fd = fopen(tagname, "rt");
    - flag = !(!fd);
    + bool flag = (fd != NULL);

    if (flag) {
    + char newtag[TAGLINE_LENGTH + 1];
    char *end;

    curr = &head;


    --
    -Andy


    ... DalekDOS v(overflow): (I)Obey (V)ision impaired (E)xterminate
    -+- MultiMail/Linux v0.52

    --- BBBS/Li6 v4.10 Toy-4
    * Origin: Prism bbs (1:261/38)
  • From mark lewis@1:3634/12.73 to Andrew Alt on Sun Aug 11 13:09:14 2019

    On 2019 Aug 09 15:01:24, you wrote to Alan Ianson:

    What's the patch do? I'll have look soonish.. :)

    Not much of anything really xD It's pretty trivial.. but thought it was worth doing. I narrowed the scope of newtag and put the fopen statement next to the declaration of the file descriptor, and made a slight change
    to
    the condition (which probably could just be "flag = (fd);" but the PR was closed so... guess I won't make any more changes to it...

    the main thing i noted was no real explanation of why the changes were made... your above says a lot more than the report on the repository but it still leaves some "why?"...

    )\/(ark

    Once men turned their thinking over to machines in the hope that this would set them free. But that only permitted other men with machines to enslave them.
    ... A high fat diet with lots of starch & a big bag of sugar helps!
    ---
    * Origin: (1:3634/12.73)
  • From Andrew Alt@1:261/38 to Mark Lewis on Mon Aug 12 14:56:36 2019
    mark lewis wrote to Andrew Alt <=-

    the main thing i noted was no real explanation of why the changes were made... your above says a lot more than the report on the repository
    but it still leaves some "why?"...

    Just a small improvement that I noticed could be done. Not being very familiar with the codebase yet (and I'm not a C or CPP expert), I didn't see any *big* improvements I could make. Plus when contributing to a new repo and communicating
    with a maintainer for the first time, I don't like to spend much time on big patches
    until I know how the review process goes on smaller things.

    --
    -Andy


    ... Beep. Invalid Input. I take only cash.
    -+- MultiMail/Linux v0.52

    --- BBBS/Li6 v4.10 Toy-4
    * Origin: Prism bbs (1:261/38)