refactor: Add caching to KnoraGroupRepo (DEV-3311) #3204
Conversation
seakayone
changed the title
seakayone
changed the title
Remove last modfying functions from GroupResponder and make group repo extend CachingRepository
seakayone
force-pushed
the
refactor/add-caching-to-group-repo
branch
from
1fc71fa to
26cc439
Compare
seakayone
changed the title
| groupAndProject <- auth.ensureSystemAdminOrProjectAdminOfGroup(user, iri) | ||
| (group, _) = groupAndProject | ||
| internal <- knoraGroupService | ||
| .updateGroupStatus(group, request.status) | ||
| .flatMap(groupService.toGroup) | ||
| .map(GroupGetResponseADM.apply) |
There was a problem hiding this comment.
nitpick: not sure if it makes sense to have for-comprehensions if we use map and flatmap in them; or vice versa, if it makes sense to use map and flatmap, if we're in a for-comprehension
There was a problem hiding this comment.
IMO this still reads better than chaining it all in one go. These lines could be also extracted to separate private methods, especially they are reused in deleteGroup methods below.
There was a problem hiding this comment.
- I am totally ok to mix for comprehension with a flatmap here and there,
- I am totally ok with a chain of flatmaps here and there
- I had my reason of why the internal has become its own name and the intermediate transformative results where in a chain of map/flatMap for my personal preference of making it clear that toGroup and Foo.apply are simply transformations of intermediate unimportant results.
- I have extracted the code duplication in a separate method and introduced intermediate result name due to popular demand. b247906
TBH, I find this kind of discussion exhausting and have a different preference for readability.
There was a problem hiding this comment.
It does look better with the flatMap refactored.
webapi/src/main/scala/org/knora/webapi/routing/Authenticator.scala
Outdated
Show resolved
Hide resolved
webapi/src/main/scala/org/knora/webapi/routing/Authenticator.scala
Outdated
Show resolved
Hide resolved
| groupAndProject <- auth.ensureSystemAdminOrProjectAdminOfGroup(user, iri) | ||
| (group, _) = groupAndProject | ||
| internal <- knoraGroupService | ||
| .updateGroupStatus(group, request.status) | ||
| .flatMap(groupService.toGroup) | ||
| .map(GroupGetResponseADM.apply) |
There was a problem hiding this comment.
IMO this still reads better than chaining it all in one go. These lines could be also extracted to separate private methods, especially they are reused in deleteGroup methods below.
…cala Co-authored-by: Marcin Procyk <[email protected]>
…cala Co-authored-by: Marcin Procyk <[email protected]>
Pull Request Checklist
Task Description/Number
Issue Number: DEV-
PR Type
Basic requirements for bug fixes and features
Does this PR introduce a breaking change?
Does this PR change client-test-data?