Re: Current issues

[Harley D. Eades III]

Patrick Leslie Polzer wrote:

> Hello Harley,
>
> patch review continued.
>
> On Thu, 03 Nov 2005 03:00:08 -0600
> "Harley D. Eades III" <address@hidden> wrote:
>
> +        if (need_commit) {
> +                if (!command_line_prompt_boolean_question 
> +("WARNING: changes were made to the disk,\n
> +are you sure you want to quit without writing your changes")) 
> +                        return 1;
> +       }
> I would write it thus:
>
> +        if (need_commit && !command_line_prompt_boolean_question ( 
> +                	"WARNING: changes were made to the disk.\n"
> +                       "are you sure you want to quit "
> +                       "without writing your changes"))
> +                return 1;
>
> But that's probably a matter of taste and others may frown on it
> because of lazy eval and less clarity.
> But at least it keeps the indent right ;)
> Please also note that the round brace should be on the previous line.
>
>  
Agreed.

> +       if (!command_line_prompt_boolean_question 
> +           ("WARNING: rescue writes all data to disk automatically,
> continue")) 
> +               return 1;
> Same here with the round brace.
> Don't we use a tab size of 8?
>
>
> -       ped_geometry_init (&probe_start_region, *dev,
> +       ped_geometry_init (&probe_start_region, (*disk)->dev,
>                           PED_MAX(start - fuzz, 0),
> -                          PED_MIN(2 * fuzz, (*dev)->length - (start -
> fuzz)));
> -       ped_geometry_init (&probe_end_region, *dev,
> +                          PED_MIN(2 * fuzz, ((*disk)->dev)->length -
> (start - f uzz)));
> +       ped_geometry_init (&probe_end_region, (*disk)->dev,
>                           PED_MAX(end - fuzz, 0),
>
> Lines way too wide.
>
>
> +       if (need_commit) 
> +               if (!command_line_prompt_boolean_question 
> +                   ("WARNING: changes were made to the disk, 
> +are you sure you want to discard them")) 
> +                       return 1;
> Same thing as some way above.
>
>
> +        if (!command_line_prompt_boolean_question 
> +                ("WARNING: are you sure you want to write all changes
> to disk") ) 
> Too wide.
>
>
> +        command_register (commands, command_create (
> +               str_list_create_unique ("commit", _("commit"), NULL),
> +               do_commit,
> +               str_list_create (
> +_("commit                        write all changes to disk"),
> +NULL),
> +               str_list_create (_(device_msg), NULL)));
> My version:
>
> +        command_register (commands, command_create (
> +                str_list_create_unique ("commit", _("commit"), NULL),
> +                do_commit,
> +                str_list_create (
> +                        _("commit                        "
> +                          "write all changes to disk"),
> +                        NULL),
> +                str_list_create (_(device_msg), NULL)));
>
>
> +/* Prompt the user to answer a yes or no question. */
> +int 
> +command_line_prompt_boolean_question (const char* prompt) {
> +        char *user_ans;
> +        StrList *possibilities = str_list_create (_("yes"), _("no"),
> NULL);
> +        user_ans = command_line_get_word (_(prompt), _("yes"),
> possibilities, 0 );
> Last line is 82 chars wide.
>
> +        if (strcmp (user_ans, _("yes")) == 0)
> By the way, is this UTF8-safe?
> I guess whole parted isn't...
> Any volunteers?
>
> Hope my review helped.
>  
It helped a lot, I am glad you took the time to look it over. 
I agree with your recommendations and will get these things fixed. 
When I do, I will post the new patch for people to review.

Thanks *very* much.
Harley :)