-
Notifications
You must be signed in to change notification settings - Fork 1
Add client ID to ROR request and switch to APIv2 #1359
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
Conversation
sven1103
left a comment
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.
Some minor but imo still notable requests
...a/life/qbic/projectmanagement/infrastructure/organisations/CachedOrganisationRepository.java
Outdated
Show resolved
Hide resolved
...a/life/qbic/projectmanagement/infrastructure/organisations/CachedOrganisationRepository.java
Outdated
Show resolved
Hide resolved
...a/life/qbic/projectmanagement/infrastructure/organisations/CachedOrganisationRepository.java
Outdated
Show resolved
Hide resolved
...ructure/src/main/java/life/qbic/projectmanagement/infrastructure/organisations/RoRentry.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Sven F. <[email protected]>
...a/life/qbic/projectmanagement/infrastructure/organisations/CachedOrganisationRepository.java
Outdated
Show resolved
Hide resolved
sven1103
left a comment
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.
Thanks @KochTobi , a huge improvement. I still have some suggestions about readability and error handling, that i would kindly ask you to have a look at.
...structure/src/main/java/life/qbic/projectmanagement/infrastructure/organisations/RorApi.java
Outdated
Show resolved
Hide resolved
...structure/src/main/java/life/qbic/projectmanagement/infrastructure/organisations/RorApi.java
Outdated
Show resolved
Hide resolved
...structure/src/main/java/life/qbic/projectmanagement/infrastructure/organisations/RorApi.java
Outdated
Show resolved
Hide resolved
...structure/src/main/java/life/qbic/projectmanagement/infrastructure/organisations/RorApi.java
Show resolved
Hide resolved
...structure/src/main/java/life/qbic/projectmanagement/infrastructure/organisations/RorApi.java
Show resolved
Hide resolved
| } | ||
|
|
||
| @Override | ||
| public String getDisplayedName() { |
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.
The method signature does not indicate that an exception is thrown. Exceptions cause side effects and need to be taken into account by the client that calls the method. Why not making the method return value Optional in the first place?
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.
There should never be an exception as there should always be a display name in the v2 ROR API.
If there is not then an exception is appropriate. I would not annotate the method as I do not think we should ever expect that to happen.
| throw new RorRequestException(e); | ||
| } | ||
|
|
||
| if (result.statusCode() == HttpStatus.NOT_FOUND_404) { |
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.
int in comparison vs an enum?
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.
That is not an enum but a static int value 😄
Co-authored-by: Sven F. <[email protected]>
Co-authored-by: Sven F. <[email protected]>
sven1103
left a comment
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.
There are some valid sonar cloud warnings left. After they have been resolved i will happily approve your PR :D
sven1103
left a comment
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.
Great 🚀
|


Replaces #1230