[
https://issues.apache.org/jira/browse/WICKET-7145?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17924542#comment-17924542
]
ASF GitHub Bot commented on WICKET-7145:
----------------------------------------
jstuyts commented on PR #1093:
URL: https://github.com/apache/wicket/pull/1093#issuecomment-2639774324
> I think we can go ahead with your current JSpecify PR and continue
improving the annotations in future work.
I'll wait for what the final decision will be: Jakarta or JSpecify.
> The only minor concern I still have with this PR is that we are adding
`@NonNull`. From my experience (and also what JSpecify recommends and large
projects like Spring are doing) `@Nullable` makes more sense in the long run.
At some point, we will have to remove all these `@NonNull` annotations. But
this should be a trivial change.
Like I said before: once everything is annotated, it is easy to decide which
of the 2 annotations can stay.
> And then there is the question of how we continue with this and what we
require in future PRs. Is our goal to completely annotate the whole Wicket
codebase? Do we add nullability annotations whenever we touch existing code? Do
we require them in PRs? Or is this a one-time effort to slightly improve the
developer experience?
A recommendation to add the annotations when adding or modifying code is the
least that can be done. Maybe split PRs: first a PR that adds the annotations
to existing code, and then a PR that contains clearer code for the
modifications because of the added annotations. This should make reviews easier.
Annotating everything would be best, but there are some tricky situations,
that force clients that can never see a `null` to work with nullable variables.
Here are a few I found so far:
* Theoretically `getComponent()` of a behavior can return `null` (for
example, call it before binding the behavior), but I think that practically
`getComponent()` is never called when the behavior is unbound. There are a
couple of options here, but I want to discuss those when I create an issue for
behaviors.
* The `IValidatable` passed to `IValidator` will not have a `null` value,
unless the validator implements `INullAcceptingValidator`.
> Developer experience improvement: nullability
> ---------------------------------------------
>
> Key: WICKET-7145
> URL: https://issues.apache.org/jira/browse/WICKET-7145
> Project: Wicket
> Issue Type: Improvement
> Components: wicket
> Affects Versions: 10.4.0
> Reporter: Johan Stuyts
> Priority: Minor
> Attachments: WICKET-7145.patch
>
>
> Knowing whether a variable can be {{null}} or not, improves the developer
> experience. A first step is to add {{@Nonnull}} to parameters that are
> checked for {{{}null{}}}.
> The patch adds {{@Nonull }}to those parameters. The following has been done:
> * The annotation has been added to base and sub types, and to some overloads.
> * Conditional nullability has been taken into account.
> ** In some methods in {{Files}} the client may pass values for other
> parameters that allows the non-{{{}null{}}} parameter to be {{{}null{}}}. It
> is assumed that clients do not do this. If a client checks if the
> non-{{{}null{}}} parameter may be {{{}null{}}}, the client can better skip
> the call.
> In some hierarchies the handling of {{null}} is inconsistent. The contract of
> the base method has to be tightened, or the implementations need to be
> changed to support {{{}null{}}}:
> * {{{}org.apache.wicket.request.Response.encodeURL{}}}: the annotations has
> only be added to the implementations in {{ServletWebResponse}} and
> {{{}WebSocketResponse{}}}.
> * {{{}org.apache.wicket.request.http.WebResponse.encodeRedirectURL{}}}: the
> same holds true as above.
> *
> {{{}org.apache.wicket.request.mapper.parameter.IPageParametersEncoder.encodePageParameters{}}}:
> the annotation has only be added to the implementation in
> {{{}UrlPathPageParametersEncoder{}}}.
> In addition bugs were found and fixed:
> * The order of the parameters to {{Checks.notNull(...)}} in
> {{{}OriginResourceIsolationPolicy{}}}.
> * The order of parameters to {{assertNull(...)}} in {{BaseWicketTester}} and
> {{{}WicketTesterTest{}}}.
> The patch is quite big, but the changes are small and simple. The changes can
> be viewed here:
> https://github.com/apache/wicket/compare/master...jstuyts:wicket:add-non-null-to-parameters
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
