clops-users

[Clops-users] new signature of the parse method

[Mikoláš J. <mik...@gm...>]

Fint and Radu has changed the parse method to return a list of errors
rather than a boolean (and only runtime exceptions are thrown).
I must say I'd have preferred a class wrapping the list:
  a) To get the previous code to compile I added isEmpty() which looks
unintuitive. isOK would look nicer.
  b) The standard toString of List is probably not desirable for
presenting to the user the aforementioned class could have some
printMe method, presenting the errors nicely.



-- 
Mikoláš Janota M. Sc.
School of Computer Science and Informatics,
University College Dublin,
Belfield,
Dublin 4,
Ireland

Re: [Clops-users] new signature of the parse method

[Fintan F. <fi...@gm...>]

The plan is to wrap it, but our initial pass at implementation was just to
use a plain list.

We discussed this on Friday and had already agreed such.

On Sun, Jul 26, 2009 at 1:23 PM, Mikoláš Janota <mik...@gm...>wrote:

> Fint and Radu has changed the parse method to return a list of errors
> rather than a boolean (and only runtime exceptions are thrown).
> I must say I'd have preferred a class wrapping the list:
>  a) To get the previous code to compile I added isEmpty() which looks
> unintuitive. isOK would look nicer.
>  b) The standard toString of List is probably not desirable for
> presenting to the user the aforementioned class could have some
> printMe method, presenting the errors nicely.
>
>
>
> --
> Mikoláš Janota M. Sc.
> School of Computer Science and Informatics,
> University College Dublin,
> Belfield,
> Dublin 4,
> Ireland
>
>
> ------------------------------------------------------------------------------
> _______________________________________________
> Clops-users mailing list
> Clo...@li...
> https://lists.sourceforge.net/lists/listinfo/clops-users
>

Re: [Clops-users] new signature of the parse method

[Radu G. <rad...@gm...>]

On Sun, Jul 26, 2009 at 3:54 PM, Fintan Fairmichael<fi...@gm...> wrote:
> We discussed this on Friday and had already agreed such.

That said, both reasons given by Mikolas are invalid.

> On Sun, Jul 26, 2009 at 1:23 PM, Mikoláš Janota <mik...@gm...>
> wrote:
>>  a) To get the previous code to compile I added isEmpty() which looks
>> unintuitive. isOK would look nicer.

If the old version was
  if (!p.parse(args)) { printUsage(); return; }
you can't simply transform it to
  if (!p.parse(args).isEmpty()) { ... }
because that would loose the error messages that where previously
reported as exceptions. If you want to not loose functionality you
must say:
  List<CLError> errors = p.parse(args);
  if (!errors.isEmpty()) {
    report(errors, System.err); // well... if report() is added to CLError
    printUsage();
    return;
  }
In fact, this is slightly better than the original because it prints
the usage information whenever the user had an error on the command
line, not only for those errors that just happened not to throw. In
this form, it seems quite natural to test whether there are any errors
by saying "!errors.isEmpty()" and isOK() would look strange.

In short, the examples are incorrect now. But they should be fixed
after these API changes are stabilized anyway.

>>  b) The standard toString of List is probably not desirable for
>> presenting to the user the aforementioned class could have some
>> printMe method, presenting the errors nicely.

With a list you say
  report(errors, System.err);
and with a wrapping class you say
  parsingResult.reportErrors(System.err);
which is not much different.

The toString() method is for debugging (although there are some *very*
rare situations when I'd use it in a different way). I don't think
anyone ever considered it for reporting the parsing errors.

So, why would we use a wrapping class? The best reason I see is that
it gives us a layer of abstraction that we could exploit later on if
we have to make changes. In other words, it gives us more freedom to
change the implementation while keeping the interface unchanged. For
example, if for some reason we notice that errors are better organized
in a tree (say, A is a child of B if A seems to be causing B) then we
can just _add_ functions to the interface that expose the tree
structure, keep reportErrors(), and *nothing* would need to change in
the user code.

Re: [Clops-users] new signature of the parse method

[Mikoláš J. <mik...@gm...>]

couple of remarks:

- for some users it might be enough to just say the usage and drop the
error messages (e.g. 'cp foo' just does that).

- it seems to me there's some additional burden on the user to know
that the parsing was ok iff the list of errors is empty

- where does the report method come from? providing and external
printing might could be useful if we wanted to provide several
standard ways of printing errors but the user has to find those
somewhere (some additional doc reading required)

all in all, do we go for the wrapper then?

m.


On Sun, Jul 26, 2009 at 9:53 PM, Radu Grigore<rad...@gm...> wrote:
> On Sun, Jul 26, 2009 at 3:54 PM, Fintan Fairmichael<fi...@gm...> wrote:
>> We discussed this on Friday and had already agreed such.
>
> That said, both reasons given by Mikolas are invalid.
>
>> On Sun, Jul 26, 2009 at 1:23 PM, Mikoláš Janota <mik...@gm...>
>> wrote:
>>>  a) To get the previous code to compile I added isEmpty() which looks
>>> unintuitive. isOK would look nicer.
>
> If the old version was
>  if (!p.parse(args)) { printUsage(); return; }
> you can't simply transform it to
>  if (!p.parse(args).isEmpty()) { ... }
> because that would loose the error messages that where previously
> reported as exceptions. If you want to not loose functionality you
> must say:
>  List<CLError> errors = p.parse(args);
>  if (!errors.isEmpty()) {
>    report(errors, System.err); // well... if report() is added to CLError
>    printUsage();
>    return;
>  }
> In fact, this is slightly better than the original because it prints
> the usage information whenever the user had an error on the command
> line, not only for those errors that just happened not to throw. In
> this form, it seems quite natural to test whether there are any errors
> by saying "!errors.isEmpty()" and isOK() would look strange.
>
> In short, the examples are incorrect now. But they should be fixed
> after these API changes are stabilized anyway.
>
>>>  b) The standard toString of List is probably not desirable for
>>> presenting to the user the aforementioned class could have some
>>> printMe method, presenting the errors nicely.
>
> With a list you say
>  report(errors, System.err);
> and with a wrapping class you say
>  parsingResult.reportErrors(System.err);
> which is not much different.
>
> The toString() method is for debugging (although there are some *very*
> rare situations when I'd use it in a different way). I don't think
> anyone ever considered it for reporting the parsing errors.
>
> So, why would we use a wrapping class? The best reason I see is that
> it gives us a layer of abstraction that we could exploit later on if
> we have to make changes. In other words, it gives us more freedom to
> change the implementation while keeping the interface unchanged. For
> example, if for some reason we notice that errors are better organized
> in a tree (say, A is a child of B if A seems to be causing B) then we
> can just _add_ functions to the interface that expose the tree
> structure, keep reportErrors(), and *nothing* would need to change in
> the user code.
>



-- 
Mikoláš Janota M. Sc.
School of Computer Science and Informatics,
University College Dublin,
Belfield,
Dublin 4,
Ireland

Re: [Clops-users] new signature of the parse method

[Radu G. <rad...@gm...>]

On Sun, Jul 26, 2009 at 10:44 PM, Mikoláš
Janota<mik...@gm...> wrote:
> - for some users it might be enough to just say the usage and drop the
> error messages (e.g. 'cp foo' just does that).

rg@rg-ucd:cfind$ cp foo
cp: missing destination file operand after `foo'
Try `cp --help' for more information.

> - it seems to me there's some additional burden on the user to know
> that the parsing was ok iff the list of errors is empty

To me it doesn't. Most people would assume that
  OK = there are no errors
Any other behavior would be surprising.

> - where does the report method come from? providing and external
> printing might could be useful if we wanted to provide several
> standard ways of printing errors but the user has to find those
> somewhere (some additional doc reading required)

>From CLError as I said. And the doc required is not "additional" at
all: The difference is only you put the method and, as you saw, from
the point of view of calling the method it looks the same.

> all in all, do we go for the wrapper then?

Yes. I believe I gave a good reason for that:

> On Sun, Jul 26, 2009 at 9:53 PM, Radu Grigore<rad...@gm...> wrote:
>> So, why would we use a wrapping class? The best reason I see is that
>> it gives us a layer of abstraction that we could exploit later on if
>> we have to make changes. In other words, it gives us more freedom to
>> change the implementation while keeping the interface unchanged. For
>> example, if for some reason we notice that errors are better organized
>> in a tree (say, A is a child of B if A seems to be causing B) then we
>> can just _add_ functions to the interface that expose the tree
>> structure, keep reportErrors(), and *nothing* would need to change in
>> the user code.

-- 
A: Because it fouls the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing on usenet and in e-mail?

Re: [Clops-users] new signature of the parse method

[Fintan F. <fi...@gm...>]

For implementing functionality like printing the errors to a PrintStream I'd
suggest that we do create a wrapper and put it there.

We should obviously still expose the list though, so my suggestion would be
that the wrapper extends List<CLError>.

For the isOK() method, perhaps successfulParse() would be a better name?


On Sun, Jul 26, 2009 at 10:44 PM, Mikoláš Janota
<mik...@gm...>wrote:

> couple of remarks:
>
> - for some users it might be enough to just say the usage and drop the
> error messages (e.g. 'cp foo' just does that).
>
> - it seems to me there's some additional burden on the user to know
> that the parsing was ok iff the list of errors is empty
>
> - where does the report method come from? providing and external
> printing might could be useful if we wanted to provide several
> standard ways of printing errors but the user has to find those
> somewhere (some additional doc reading required)
>
> all in all, do we go for the wrapper then?
>
> m.
>
>
> On Sun, Jul 26, 2009 at 9:53 PM, Radu Grigore<rad...@gm...>
> wrote:
> > On Sun, Jul 26, 2009 at 3:54 PM, Fintan Fairmichael<fi...@gm...>
> wrote:
> >> We discussed this on Friday and had already agreed such.
> >
> > That said, both reasons given by Mikolas are invalid.
> >
> >> On Sun, Jul 26, 2009 at 1:23 PM, Mikoláš Janota <
> mik...@gm...>
> >> wrote:
> >>>  a) To get the previous code to compile I added isEmpty() which looks
> >>> unintuitive. isOK would look nicer.
> >
> > If the old version was
> >  if (!p.parse(args)) { printUsage(); return; }
> > you can't simply transform it to
> >  if (!p.parse(args).isEmpty()) { ... }
> > because that would loose the error messages that where previously
> > reported as exceptions. If you want to not loose functionality you
> > must say:
> >  List<CLError> errors = p.parse(args);
> >  if (!errors.isEmpty()) {
> >    report(errors, System.err); // well... if report() is added to CLError
> >    printUsage();
> >    return;
> >  }
> > In fact, this is slightly better than the original because it prints
> > the usage information whenever the user had an error on the command
> > line, not only for those errors that just happened not to throw. In
> > this form, it seems quite natural to test whether there are any errors
> > by saying "!errors.isEmpty()" and isOK() would look strange.
> >
> > In short, the examples are incorrect now. But they should be fixed
> > after these API changes are stabilized anyway.
> >
> >>>  b) The standard toString of List is probably not desirable for
> >>> presenting to the user the aforementioned class could have some
> >>> printMe method, presenting the errors nicely.
> >
> > With a list you say
> >  report(errors, System.err);
> > and with a wrapping class you say
> >  parsingResult.reportErrors(System.err);
> > which is not much different.
> >
> > The toString() method is for debugging (although there are some *very*
> > rare situations when I'd use it in a different way). I don't think
> > anyone ever considered it for reporting the parsing errors.
> >
> > So, why would we use a wrapping class? The best reason I see is that
> > it gives us a layer of abstraction that we could exploit later on if
> > we have to make changes. In other words, it gives us more freedom to
> > change the implementation while keeping the interface unchanged. For
> > example, if for some reason we notice that errors are better organized
> > in a tree (say, A is a child of B if A seems to be causing B) then we
> > can just _add_ functions to the interface that expose the tree
> > structure, keep reportErrors(), and *nothing* would need to change in
> > the user code.
> >
>
>
>
> --
> Mikoláš Janota M. Sc.
> School of Computer Science and Informatics,
> University College Dublin,
> Belfield,
> Dublin 4,
> Ireland
>

Re: [Clops-users] new signature of the parse method

[Radu G. <rad...@gm...>]

On Mon, Jul 27, 2009 at 11:43 AM, Fintan Fairmichael<fi...@gm...> wrote:
> We should obviously still expose the list though, so my suggestion would be
> that the wrapper extends List<CLError>.

I would prefer if we simply have a method like
  public class ParseResult {
    public void List<CLError> listErrors() { ...}
  }
This way we can change (if needed in future versions) the internal
representation easily to anything that can still be *converted* to a
list.

Re: [Clops-users] new signature of the parse method

[Radu G. <rad...@gm...>]

On Mon, Jul 27, 2009 at 1:18 PM, Radu Grigore<rad...@gm...> wrote:
> On Mon, Jul 27, 2009 at 11:43 AM, Fintan Fairmichael<fi...@gm...> wrote:
>> We should obviously still expose the list though, so my suggestion would be
>> that the wrapper extends List<CLError>.
>
> I would prefer if we simply have a method like [...]

Actually you are right, we can implement Iterable<CLError> too.

I don't know why I was thinking that List<CLError> is a class.