-
Notifications
You must be signed in to change notification settings - Fork 238
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Java] replace withSecureMode by requireClassRegistration API #768
[Java] replace withSecureMode by requireClassRegistration API #768
Conversation
@tisonkun Could you help review this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While withSecureMode(false)
is vague and even ambiguous, is it possible with use requireClassRegistration
to replace all the usage of withSecureMode
instead and document requireClassRegistration(false)
's safety requirement (i.e., "your environment is indeed secure")?
I don't know if withSecureMode
has other meaning - it reads vague and ambiguous to me. Here we can expose the real effect requireClassRegistration
that is not an internal concept but what users modify the option need to know.
Good suggestion, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. One comment inline that can be improved later.
050d731
to
ebef6de
Compare
What do these changes do?
remove requireClassRegistration API
Related issue number
Closes #769
Check code requirements