-
Notifications
You must be signed in to change notification settings - Fork 31
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
location-retrieval: Alignment of errors with Commonalities #221
Conversation
Alignment of errors with Commonalities
🦙 MegaLinter status: ✅ SUCCESS
See detailed report in MegaLinter reports |
value: | ||
status: 400 | ||
code: INVALID_ARGUMENT | ||
message: "Invalid argument" | ||
MaxAgeIssue: | ||
message: Client specified an invalid argument, request body or query param. |
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.
In theory this also applies to request headers. Maybe we can simplify the sentence, keep it more generic ?
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.
Hello @alpaycetin74
I used the the template from commonalities here - line 146.... so probably we cannot change this.
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.
Also, the value for message in examples are not normative, only the code. Implementation may return any other, even in different language
examples: | ||
GENERIC_403_PERMISSION_DENIED: | ||
summary: Generic Permission Denied | ||
description: Permission denied. OAuth2 token access does not have the required scope or when the user fails operational security |
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.
Maybe reword as "OAuth2 token does not have access to the required scope, or the user fails operational security" ?
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 previous. I took this from commonalities here - line 192
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, apart from reference to mobile
message: 'The specified resource is not found' | ||
deviceNotFound: | ||
LOCATION_RETRIEVAL_404_UNABLE_TO_LOCATE_DEVICE: | ||
summary: The location server is not able to locate the mobile |
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.
Maybe better "device" instead of "mobile"
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.
Yes - fixed.
Changed mobile to device for error 404
Fixed last @jlurien comment. |
I agree, thank you. |
@jlurien May I ask you to take a look on this one. I guess we need to move quick on this one for the meta-release. Thanks |
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.
We may need to add support for 422 UNIDENTIFIABLE_DEVICE both this one and verification, but we can ammend that once Commonalities issues is addressed
Added - If you think it's too early I can remove but Rafal is working on the PR. |
What type of PR is this?
Add one of the following kinds:
What this PR does / why we need it:
Alignement of errors according to Commonalities guideline.
Which issue(s) this PR fixes:
Fixes #219
Special notes for reviewers:
Changelog input
Additional documentation
This section can be blank.