Code review comment for lp:~riddlepl/granite/trunk

Revision history for this message
Victor Martinez (victored) wrote :

This is one of those unnecessary ABI/API breaks IMHO. As I told darklin on IRC, making the default option context a protected read-only property is enough. For instance:

// Add the property
private OptionContext op_context;
protected OptionContext option_context {
   get {
       if (op_context == null)
          op_context = new OptionContext (exec_name ?? "");
       return op_context;
   }
}

After that, you would remove the code in Granite.Application.run() that creates a new context.

How is this supposed to be used? For example, in Noise:

// Noise.App constructor
public App () {
    option_context.add_main_entries (app_specific_entries);
    [...]
}

When Granite.Application.run() is called, all our options would be part of the default context and parsed accordingly. Our app's options would be later applied in activate().

The print-version option is a neat addition! Please use message() instead of stdout.print() and it will be perfect.

review: Needs Fixing

« Back to merge proposal