You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "chenjian2664 (via GitHub)" <gi...@apache.org> on 2024/12/17 06:55:09 UTC
[PR] Add nonnull annotations for public constructor [polaris]
chenjian2664 opened a new pull request, #573:
URL: https://github.com/apache/polaris/pull/573
<!--
Possible security vulnerabilities: STOP here and contact security@apache.org instead!
Please update the title of the PR with a meaningful message - do not leave it "empty" or "generated"
Please update this summary field:
The summary should cover these topics, if applicable:
* the motivation for the change
* a description of the status quo, for example the current behavior
* the desired behavior
* etc
PR checklist:
- Do a self-review of your code before opening a pull request
- Make sure that there's good test coverage for the changes included in this PR
- Run tests locally before pushing a PR (./gradlew check)
- Code should have comments where applicable. Particularly hard-to-understand
areas deserve good in-line documentation.
- Include changes and enhancements to the documentation (in site/content/in-dev/unreleased)
- For Work In Progress Pull Requests, please use the Draft PR feature.
Make sure to add the information BELOW this comment.
Everything in this comment will NOT be added to the PR description.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add nonnull annotations for `PolarisEntityManager` public constructor [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain merged PR #573:
URL: https://github.com/apache/polaris/pull/573
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add nonnull annotations for `PolarisEntityManager` public constructor [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #573:
URL: https://github.com/apache/polaris/pull/573#issuecomment-2556101346
I believe it's appropriate to apply the `@Nonnull` annotation to these parameters. By checking these fields, they are indeed expected to be non-null. This annotation will make the intent explicit and enhance both code clarity and safety. With that, +1 on the PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add nonnull annotations for `PolarisEntityManager` public constructor [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #573:
URL: https://github.com/apache/polaris/pull/573#issuecomment-2563903915
I suppose using the "non-null by default" convention in APIs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add nonnull annotations for `PolarisEntityManager` public constructor [polaris]
Posted by "chenjian2664 (via GitHub)" <gi...@apache.org>.
chenjian2664 commented on PR #573:
URL: https://github.com/apache/polaris/pull/573#issuecomment-2556392001
@adutra Thanks for the reply.
> I think that instead we should assume that in all Polaris code, parameters and return types are non-null by default
In this case, explicit check would make code more safe just as @flyrain mentioned.
> and only annotate the nullable ones.
The current code base seems do the both the explicit check for null and nonnull in most places
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add nonnull annotations for `PolarisEntityManager` public constructor [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #573:
URL: https://github.com/apache/polaris/pull/573#issuecomment-2568118411
Using a global setting for parameters is a good idea, especially for modules dedicated to Polaris, like `polaris-service`. However, I’m concerned about how this approach would work with shared modules like `polaris-core`, which might be used by other projects.
I believe this needs further discussion on the dev list. Could you please file an issue to track this? Thanks! @adutra
It doesn't block this PR anyway. I will merge it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add nonnull annotations for `PolarisEntityManager` public constructor [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #573:
URL: https://github.com/apache/polaris/pull/573#issuecomment-2553302328
@chenjian2664 I'm not sure we want to go in this direction. I think that instead we should assume that in all Polaris code, parameters and return types are non-null by default, and only annotate the nullable ones. WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add nonnull annotations for `PolarisEntityManager` public constructor [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #573:
URL: https://github.com/apache/polaris/pull/573#issuecomment-2562515446
@flyrain and @chenjian2664 I don't mind adding these annotations, but if we think globally about Polaris code, and assuming that, since the API is biased towards non-nullability by default, about 80% of method arguments and method return types are non-null, annotating each and every one of them will quickly become a visual clutter.
Instead, there are better options to mark an API as "non-null by default", e.g. using JSR 305 annotation `@ParametersAreNonnullByDefault` placed at package level. Most IDEs are capable of understanding that and would automatically enforce non-nullability everywhere.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org