-
Notifications
You must be signed in to change notification settings - Fork 632
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
Add rfc80 clean arch #11396
Add rfc80 clean arch #11396
Conversation
4d51870
to
8dc093b
Compare
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
Will the services in |
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! Just added a minor question about an empty interface
@@ -0,0 +1,4 @@ | |||
package org.cbioportal.infrastructure.repository.clickhouse.studyview; | |||
|
|||
public interface ClickhouseStudyViewFilterMapper { |
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.
Is this empty interface intentional?
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.
Yea, because it is mapped to the StudyViewFilterMapper.xml that has all the common logic for the studyViewFilter
|
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 great! Thank you! As mentioned on standup - would be nice to have some docs on the design decisions but can be done in follow-up PR
Particularly we should prolly update these:
Updated RFC80 code to utilize new architecture.
I have not refactored all the code, but have deprecated most of it.
I moved everything under domain package