Keyboard Shortcuts

File
u :up to issue
m :publish + mail comments
M :edit review message
j / k :jump to file after / before current file
J / K :jump to next file with a comment after / before current file
Side-by-side diff
i :toggle intra-line diffs
e :expand all comments
c :collapse all comments
s :toggle showing all comments
n / p :next / previous diff chunk or comment
N / P :next / previous comment
<Up> / <Down> :next / previous line
<Enter> :respond to / edit current comment
d :mark current comment as done
Issue
u :up to list of issues
m :publish + mail comments
j / k :jump to patch after / before current patch
o / <Enter> :open current patch in side-by-side view
i :open current patch in unified diff view
Issue List
j / k :jump to issue after / before current issue
o / <Enter> :open current issue
# : close issue
Comment/message editing
<Ctrl> + s or <Ctrl> + Enter :save comment
<Esc> :cancel edit
Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(75)
Issues Repositories Search
Open Issues | Closed Issues | All Issues | Sign in with your Google Account to create issues and add comments

Issue 110040044: go.tools/go/loader: Don't print errors, only collect them.

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 6 months ago by gmk
Modified:
11 years, 6 months ago
Reviewers:
gri, adonovan
CC:
golang-codereviews
Visibility:
Public.
go.tools/go/loader: Don't print errors, only collect them.

Patch Set 1 #

Patch Set 2 : code review 110040044: go.tools/go/loader: Don't print errors, only collect them. #

Patch Set 3 : diff -r 93422c3d1033 https://code.google.com/p/go.tools #

Created: 11 years, 6 months ago
Download [raw] [tar.bz2]
Unified diffs Side-by-side diffs Delta from patch set Stats (+7 lines, -9 lines) Patch
M go/loader/loader.go View 1 2 2 chunks +7 lines, -9 lines 0 comments Download
Total messages: 5
|
gmk
11 years, 6 months ago (2014年06月19日 18:52:00 UTC) #1
Sign in to reply to this message.
adonovan
The default behaviour has always been to print errors to standard error, and this is ...
11 years, 6 months ago (2014年06月19日 18:55:55 UTC) #2
The default behaviour has always been to print errors to standard error,
and this is sensible for most applications. If your application wants to
suppress them, then add this line:
 conf.TypeChecker.Error = func(error){}
to your configuration, before calling conf.Load().
Sign in to reply to this message.
gmk
The default behavior in go/types isn't to print them, so why should go/loader? Whatever it ...
11 years, 6 months ago (2014年06月19日 19:05:53 UTC) #3
The default behavior in go/types isn't to print them, so why should go/loader? 
Whatever it has always been is not relevant. I don't agree that printing is the
sensible for most applications; for most applications this is spam. The
sensible behavior for any library is to never print anything unless explicitly
requested to do so.
Sign in to reply to this message.
adonovan
On 2014年06月19日 19:05:53, gmk wrote: > The default behavior in go/types isn't to print them, ...
11 years, 6 months ago (2014年06月19日 19:18:23 UTC) #4
On 2014年06月19日 19:05:53, gmk wrote:
> The default behavior in go/types isn't to print them, so why should go/loader?
The default behaviour in go/types is to abort type checking and return the first
error from (*Checker).Files; it does not silently accumulate them, as this would
create a trap of forgetting to report them.
> Whatever it has always been is not relevant. 
It is certainly relevant to all existing clients, since you are changing their
behaviour.
> I don't agree that printing is the sensible for most applications; for most
applications this is spam.
That has not been my experience with command-line tools built atop go/loader.
The only application for which I have wanted to suppress this behaviour is
godoc, since it reports type errors via its HTTP interface.
It uses the one-line trick I mentioned before, which works nicely.
> The sensible behavior for any library is to never print anything unless
explicitly
> requested to do so.
Usually, but with error reporting, you have many choices:
- fail the entire operation at the first error (go/types' default)
- force the user to supply an error handler
- accumulate errors and hope the user remembers to look at them
- print them by default, but permit other strategies.
The advantage of the last one is that it provides the most information with the
least configuration, and fails safe. (It is also quite handy when a program
panics, since the code to print the accumulated errors may not have been reached
yet, but the default error handler will have already printed them.)
Sign in to reply to this message.
gmk
go/loader doesn't silently accumulate errors unless you explicitly set AllowErrors=true. Of course it is not ...
11 years, 6 months ago (2014年06月19日 19:46:12 UTC) #5
go/loader doesn't silently accumulate errors unless you explicitly set
AllowErrors=true.
Of course it is not desirable to change existing behavior, but at this point I
think it is more important to get the right behavior. The docs make it clear
that this package is unstable.
I can understand that printing errors might be a sensible default for
command-line tools, but we shouldn't assume that all clients are like that. 
Even if I was writing a command-line tool, I would rather have the
responsibility of printing errors at the appropriate time and place than have it
done for me.
Does anything in the standard library print its errors by default? It's
paranoid to worry that users will forget to check for errors. This is Go - The
Language In Which We Check For Errors. :-)
Sign in to reply to this message.
|
Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b

AltStyle によって変換されたページ (->オリジナル) /