Skip to content
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

Merged
merged 9 commits into from
Feb 20, 2025
Merged

Add rfc80 clean arch #11396

merged 9 commits into from
Feb 20, 2025

Conversation

haynescd
Copy link
Collaborator

@haynescd haynescd commented Feb 18, 2025

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

@haynescd haynescd self-assigned this Feb 18, 2025
@haynescd haynescd force-pushed the add-rfc80-clean-arch branch from 4d51870 to 8dc093b Compare February 18, 2025 18:59
Copy link
Contributor

@fuzhaoyuan fuzhaoyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fuzhaoyuan
Copy link
Contributor

Will the services in studyview folder be distributed into each use case as well?

Copy link
Member

@onursumer onursumer left a 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 {
Copy link
Member

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?

Copy link
Collaborator Author

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

Copy link
Member

@inodb inodb left a 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:

@haynescd haynescd merged commit be86cc3 into master Feb 20, 2025
24 of 25 checks passed
@haynescd haynescd deleted the add-rfc80-clean-arch branch February 20, 2025 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants