-
Notifications
You must be signed in to change notification settings - Fork 51
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
On error validation #289
base: main
Are you sure you want to change the base?
On error validation #289
Conversation
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.
Looks good for the most part. Maybe some room for minor improvements.
|
||
invalidParamsCheck(type, WebSocketConstants.ON_OPEN, WebSocketConstants.ON_OPEN_PARAM_OPT_TYPES, | ||
WebSocketConstants.RAW_ON_OPEN_PARAM_OPT_TYPES, WebSocketConstants.DIAGNOSTIC_CODE_ON_OPEN_INVALID_PARAMS, | ||
Collections.emptySet(), Collections.emptySet(), unit, diagnostics); |
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.
might be a bit more space-efficient to pass in null instead of empty sets here and then have a check in the function?
Collections.emptySet(), Collections.emptySet(), unit, diagnostics); | ||
invalidParamsCheck(type, WebSocketConstants.ON_CLOSE, WebSocketConstants.ON_CLOSE_PARAM_OPT_TYPES, | ||
WebSocketConstants.RAW_ON_CLOSE_PARAM_OPT_TYPES, WebSocketConstants.DIAGNOSTIC_CODE_ON_CLOSE_INVALID_PARAMS, | ||
Collections.emptySet(), Collections.emptySet(), unit, diagnostics); |
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.
same as above for empty sets
Diagnostic diagnostic2 = createDiagnostic(genMandTypeCounter.get(genParamType), unit, | ||
diagMsg, WebSocketConstants.DIAGNOSTIC_CODE_DUP_PARAMS_TYPES); | ||
diagnostics.add(diagnostic2); | ||
continue; |
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.
redundant continue
public static final Set<String> ON_ERROR_PARAM_OPT_TYPES = new HashSet<>(Arrays.asList("jakarta.websocket.Session")); | ||
public static final Set<String> RAW_ON_ERROR_PARAM_OPT_TYPES = new HashSet<>(Arrays.asList("Session")); |
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.
I suspect if you pull the latest changes from main, you'll be able to use the defined strings for these hashsets.
Resolves #249