Community
Participate
Working Groups
Created attachment 103325 [details] Fix correcting javadoc and condition. Build ID: I20080502-0100 By default parameter cannot be null so we do not have to say it again. Condition checking if argument is null is incorrect.
Personally, I think it's helpful to note that the argument cannot be null but I don't know what Platform/UI's standards are.
(In reply to comment #1) > Personally, I think it's helpful to note that the argument cannot be null but I > don't know what Platform/UI's standards are. Agree. Boris, what's the "in" way to denote this?
We actually have a document [1] that gives defaults (if we don't say anything, null is not allowed). However, typical clients have so many things to worry about that we should help them where we can, i.e. we should not make it a requirement to read the document in order to understand our API contracts. +1 for making constraints about null explicit. But also +1 for changing the manually thrown NPE into an Assert, or even better into an IllegalArgumentException: You want to make it clear whose fault it is when an exception happens. An NPE is usually interpreted as an error in the callee, whereas an IllegalArgumentException clearly points to the caller as the culprit.
oops [1] http://www.eclipse.org/articles/article.php?file=Article-API-Use/index.html
(In reply to comment #3) > But also +1 for changing the manually thrown NPE into an Assert, or even better > into an IllegalArgumentException: You want to make it clear whose fault it is > when an exception happens. An NPE is usually interpreted as an error in the > callee, whereas an IllegalArgumentException clearly points to the caller as the > culprit. If we want an IAE then I guess it should be changed to call isLegal instead of isNotNull.
Created attachment 103531 [details] Fix Javadoc untouched. Test case created. Condition corrected - please note that the most important issue here is invalid condition (method argument is not checked in original code), therefore I'd like to see this included in current RC or in 3.4.1.
Created attachment 107477 [details] Fix Ok, back to work. I would like to point that the problem with assertion message and javadoc is not the most important thing here. Note that the condition statusListLabelProvider != null is not checking the parameter, but some internal field. There are two tests that should be passed: testNullLabelProvider - to be sure that one cannot pass null there testNonNullLabelProvider - to be sure that custom label provider is really used Kevin, could you review?
Created attachment 107478 [details] mylyn/context/zip
(In reply to comment #7) > Note that the condition statusListLabelProvider != null is not checking the > parameter, but some internal field. D'oh right! Its checking the existing field, not the parameter which will be assigned to the field. > Kevin, > could you review? Comments: 1) Minor: javadoc should be "<code>null</code>" 2) Why are you doing Assert.isLegal(labelProvider != null) vs. Assert.isNotNull(labelProvider); Assert.isNotNull() is more explicit/specific (i.e. matches the comment). It's also I believe the "in" way; if you look for references of each you'll notice 3x isNotNull vs. isLegal. (note: reminder that if we change this then test must also be changed to match)
Created attachment 107733 [details] Slightly modified fix about IAE: I was inspired by comment 3: "You want to make it clear whose fault it is when an exception happens. An NPE is usually interpreted as an error in the callee, whereas an IllegalArgumentException clearly points to the caller as the culprit". As I said - I think that a kind of exception is a subject for rather academic discussion, so feel free to change it.
I think you can regard isNotNull() as a more precise isLegal(). Its a subset of the cases. Boris' comment was that an NPE is usually a system error (our bug), while as both isNotNull() and isLegal() identify the parameter as not matching the specification (the caller's bug). I'm not sure why Boris specifically mentioned IllegalArgumentException vs. Assert. Its true that isLegal() throws IllegalArgumentException, and isNotNull() throws AssertionFailedException(), but I'm not sure that makes a practical difference. isNotNull seems to me the right Assert but I'm not really fussed either way, they both have the right quality.
Spoke with Paul about best practice on isLegal(param != null) or isNotNull(param). He tends to the former since it explicitly states its for argument exceptions. In practice though it looks to me we always use the latter. In fact I only found 47 references to isLegal(* != null) and those were all in texteditor! I'm fine with either.
Released to HEAD. Thanks for the patch and test.
verified by running tests (cause there is test case) and by code inspection
