-
Notifications
You must be signed in to change notification settings - Fork 0
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
네트워크 모델 변환 함수 생성 #312
네트워크 모델 변환 함수 생성 #312
Conversation
// temp test | ||
implementation(project(":core:network")) | ||
implementation(project(":core:database")) | ||
implementation(project(":core:model")) // TODO : revisit | ||
} |
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.
이게 implementation이면 생기는 문제점
core:model <- core:network <- core:data
NIA에서도 그렇고 우리도 다 완성되면 이런 의존성이 생긴다.
그러면, core:data는 core:network에 의존하는 것 만으로도 core:model의 data class나 함수들을 가져올 수 있다.
그런데, api가 아니라 implementation으로 가져오면 그렇게 못한다.
지금 해놓은 것처럼 core:model도 명시적으로 가져와야 한다.
나중에 필요한 종속 구성에 맞춰 바꿔야 한다.
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.
실제로 찾아보니까 (Developers 문서)
api와 implementation의 차이가 딱 저거로 구분되는 듯 한데...
설명만 봤을 때는 api를 썼을 때의 장점은 딱히 없어보인다. (빌드 시간의 관점에서)
장점이 있다면 프로그램을 짤 때 있는 장점 정도..? (위 코멘트가 implementation의 문제점 = api의 장점)
그럼에도 NIA는 위 코멘트 같은 경우 뿐만 아니라 모듈간 종속에 있어서 거의 api를 쓰고 있다.
근데 그렇게 하면 '빌드 시간 개선'이라는 처음 목적에 어긋나지는 않나 싶기도 한다..
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.
아하 빌드속도가 다르구나..
일단 빌드 시간이 늘어나진 않을듯? core:model같은건 결국 feature에까지 전달되어야 하는 클래스들을 가지니, build.gradle.kts(:core:network)에서 api(projects.core.model)하지 않더라도 온갖 모듈에서 implementation(projects.core.model)을 해줘야 할 거고, 그럼 컴파일되는 경우의 수는 동일할 것 같아서
api를 썼을 때의 장점은 딱히 없어보인다
내가 생각한 장점은 이래
- 여러 곳에서 implementation 쓰는 것보다 의존성 그래프가 단순해진다
- 중복코드(implementation)를 줄일 수 있다
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.
NIA는 위 코멘트 같은 경우 뿐만 아니라 모듈간 종속에 있어서 거의 api를 쓰고 있다.
NIA가 모듈 간 종속에 있어서 api를 쓰는 경우도 있는데, core 끼리만 적당히 쓰는 듯..? feature 모듈에서 다른 모듈에 의존할 때에는 무조건 implementation만 씀. core:model처럼 온갖 곳에서 쓰이는 모듈은 확실히 api로 의존되는 경우가 많고, core:designsystem같은건 implementation으로 의존되는 경우가 많네.
'빌드 시간 개선'이라는 처음 목적에 어긋나지는 않나 싶기도 한다
core:model처럼 온갖 곳에서 쓰이는 모듈은, 수정 시 빌드 시간이 길어진다는 단점을 피할 수 없는 것 같기도..
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.
core:model <- core:network 인데, core:network 에서 외부에 공개하는 부분(예: API의 응답 타입)에 core:model 이 반드시 포함되고 있으니까 api 를 쓰지 않으면 안 될 거야
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.
core:network 에서 core:model 을 api 로 들고 있지 않고 implementation 으로만 들고 있으면, core:network 를 참조하는 다른 모듈에서 core:network랑 core:model 을 둘 다 종속성으로 줘야 하는데, 이러면 굳이 다른 모듈에 core:model의 종속성을 명시해 줘야 하니까 불필요하지
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.
NIA를 기준으로 따라가자
buildingNameEng = this.buildingNameEng ?: "", | ||
coordinate = this.locationInDMS.toExternalModel(), // TODO : locationInDMS랑 locationinDecicmal이 내려오고 있다.. | ||
) |
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.
이거 locationInDMS랑 locationInDecimal 중 DMS 쓰는게 맞는건가?
(예를 들어 locationInDMS = (37.46073, 126.95452)면 locationInDecimal = (489941.0, 1100367.0))
뒤에거가 어디 쓰이냐에 따라 둘다 필요할 수도 있을 듯 단순히 비율 차이는 아닌 듯 해서
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.
현재 locationInDMS만 쓰고 있으므로,
그대로 두고 주석만 지우면 될 듯
|
||
fun ColorDto.toExternalModel() = LectureColor( | ||
foregroundString = this.fgRaw ?: "#000000", | ||
backgroundString = this.bgRaw ?: "#FFFFFF", | ||
) | ||
// TODO : 검은 바탕에 흰 글씨를 default로 해놨는데, 추후 revisit |
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.
ColorDto의 도메인 모델인 LectureColor를 non-nullable로 하는 과정에서 디폴트 값이 필요했는데,
일단 그 값을 검은 바탕에 흰 글씨로 해놓음
|
||
// TODO : 논의 중, RemoteConfig라는 model 자체가 필요 없다는 얘기도 있었다. | ||
fun RemoteConfigDto.toExternalModel() = RemoteConfig( | ||
friendsBundleSrc = reactNativeBundleSrc?.src?.get("android"), | ||
vacancyNotificationBannerEnabled = vacancyBannerConfig.visible, | ||
sugangSNUUrl = vacancyUrlConfig.url, | ||
settingPageNewBadgeTitles = settingsBadgeConfig.new, | ||
disableMapFeature = disableMapFeature ?: false, | ||
) |
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.
RemoteConfig라는 model 자체가 필요 없다는 얘기가 있었던 이유
RemoteConfig는 그 내부에서 값을 처리해서 기본 타입 (String 이런거)의 값만 외부로 주기 때문에,
똑같이 기본 타입의 값만 외부로 주는 레포지토리를 생각해보면
레포지토리는 네트워크 모델을 쓸 수 있기 때문..
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.
첫 번째 RemoteConfig는 새로 만드는 model 말한거고
두 번째 RemoteConfig는 지금 있는 리모트컨피그 받아오고 가공하는 클래스 말한거지?ㅋㅋㅋㅋ
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.
아 그러네 맞아 ㅋㅋㅋㅋㅋ
TagDto 관련 현재 tag 목록을 가져오는 과정 (내가 이해한게 맞다면...)
그래서 core:network의 model에는 TagDto가 있는게 아니라, GetTagListResults가 있다. 그리고 core:model에 Tag라는 data class를 넣을 예정이다. 그러면, GetTagListResults -> Tag 변환 함수는 필요 없는거 아닐까..? |
|
||
abstract class OriginalLecture( | ||
data class OriginalLecture( | ||
override val id: String, |
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.
얘랑 TimetableLecture abstract로 되어 있길래 바꿨어
|
||
fun LectureDto.toExternalModel(): Lecture { | ||
return when (this.color == ColorDto()) { // TODO : revisit (true이면 OriginalLecture, false이면 TimeTableLecture 반환) | ||
true -> OriginalLecture( | ||
id = this.id, // TODO : 모지 이거 id랑 lecture_id 중에 뭐가 필요한거지..? | ||
classification = this.classification, |
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.
이 파일에서 revisit 하고 싶은 점
- 어떤 도메인 모델로 바꿀지 (OriginalLecture, TimetableLecture) 결정하는 로직이 저게 괜찮은가
- id와 lecture_id를 이해 못해서 일단 type이 맞는대로 끼워넣어 놓음..
- 더 뭔가(ui, viewmodel, repository 등)를 만들기 전에 잘 작동하는지 테스트 할 수 있으면 좋을 듯
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.
나도 헷갈려서 전에 딱 정리해둠
https://wafflestudio.slack.com/archives/C0PAVPS5T/p1718805215296379?thread_ts=1718796714.629209&cid=C0PAVPS5T
어떤 API의 응답이냐에 따라 원본 강의 id
가 다른 필드에 내려와
- POST /v1/search_query에서는
_id
- GET /v1/tables/{tableId}, GET /v1/tables/recent에서는
lecture_id
POST /v1/search_query에서는 lecture_id 자체가 내려오지 않음. 강의 id
라는 게 2개씩이나 필요하지 않은 상황이니까.
즉, lecture_id가 null이라면 OriginalLecture라고 할 수 있다
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.
5 -> Day.SATURDAY | ||
else -> Day.SUNDAY | ||
}, |
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.
이것도 enum Day에 혹시 다른 값이 내려올 때를 대비해서 Day.Default 이런거 추가할까?
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.
음 난 없어도 된다고 생각해
Notification type같은 건 나중에 다른 값이 추가될 여지가 충분한데, 이건 요일이다보니 다른 값이 추가되진 않을거고
오히려 안 쓰이는 케이스가 하나 더 생김으로써 불필요한 예외처리가 많아질 수 있을 듯..?
예를 들어 요일피커같은거에서 when(Day형 변수) 문을 쓴다고 하면, Day.Default에 대한 동작을 정의하지 않으면 컴파일 에러날거야. when 문은 모든 케이스에 대해 처리해야 하기 때문. 이 때 Day.Default에 대한 동작을 정하기가 애매하단거지
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.
음 그러네 쓰는 곳에서 애매하겠구나
), | ||
place = Place( | ||
name = this.place, | ||
building = null | ||
) | ||
) |
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.
이 부분에서 의문이 생겼는데, (슬랙)
만약 이렇게 하게 된다면
나중에 강의 상세 들어갈 때 v1/building를 쏘고, GetBuildingsResponse가 내려올텐데 Building.kt에 있는 LectureBuildingDto.toExternalModel()를 이용할 수 있다.
그렇게 해서 Building을 얻어서 저 place의 building 자리를 채울 수 있다.
우리 TableTheme/BuiltInTheme/CustomTheme 얘기했던게 |
totalCredit = this.totalCredit ?: 0L, | ||
theme = // TODO : 이 부분에서 문제 발생 | ||
isPrimary = this.isPrimary, |
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.
이거 논의할때 theme으로 색깔 얻는거 ui에서 그릴거니까 거기로 옮기자고 했는데 (context가 무진장 많이 필요)
그렇게 하니까 TimeTable의 변환 함수를 못만드네... 이거 어떻게 하지
지금 theme 자리에 BuiltInTheme/CustomTheme 둘 중 하나가 와야 하고 만약 CustomTheme인 경우... 어떻게 하지?
일단 생각한 방법은 CustomTheme에서 colors라는 필드를 아예 없애기?
어차피 ui에서 바꿀거면 필요 없기도 하고
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.
으아아악 CustomTheme이라는 모델에 colors는 무조건 있어야 자연스러울거같은데
theme 관련된건 내가 커스텀테마 feature모듈 만들면서 또는 그 전에 리비짓할게...
일단 제공테마만 생각한다고 하면 theme: Int로 두고 TableDto.theme 그대로 가져오면 될거야
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 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.
푸시했어
c2226c7
추가 의문: 데이터베이스 모델 <-> 도메인 모델 변환 함수는 필요한건가..? |
fun TableDto.toExternalModel() = TimeTable( | ||
id = this.id, | ||
courseBook = CourseBook( | ||
semester = this.semester, | ||
year = this.year, | ||
), | ||
title = this.title, | ||
lectureList = this.lectureList.map { it.toTimetableLecture() }, | ||
totalCredit = this.totalCredit ?: 0L, | ||
theme = this.theme | ||
isPrimary = this.isPrimary, |
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.
확장함수들에 this 없으면 더 깔끔할것같아
컴파일러가 다 알아서 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.
어 근데 이제보니 NIA는 toExternalModel이 아니라 asExternalModel이네ㅋㅋㅋ 그게그거겠지머
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.
아 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.
어 근데 이제보니 NIA는 toExternalModel이 아니라 asExternalModel이네ㅋㅋㅋ 그게그거겠지머
헐 ㅋㅋㅋㅋㅋ
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.
832104d
this 제거
엉 변환함수는 ThemeDto에 메서드로 들어있어서 그것만 data 모듈로 옮겨놨어 f4fa74d pull 땡겨줘 |
필요하지? 데이터베이스는 다른 계층이고, 두 모델은 다른 모델이니까 NIA도 보면 asEntity() 이런 변환함수들 있네 |
나 이해를 잘 못했는데ㅋㅋㅋㅋ... TagDto -> Tag 가 필요없고 GetTagListResults -> Tag가 필요하다는 뜻이야? |
데이터베이스 모델 <-> 도메인 모델 변환 함수 |
내 생각대로면 GetTagListResults -> Tag도 필요 없을 것 같아
이런 이유로..? 모델을 따로 두는 것까지는 의미가 있지만 |
아아아 이해했어 흠 그러게 TagDto라는게 따로 없어서 레포지토리의 메서드에서 List만 리턴하면 되는구나 |
굿굿 내일 다같이 다시 보고 머지하자 수고했당 |
placeName = place | ||
) | ||
|
||
// TODO : 제대로 되는지 테스트 해보면 좋을 듯 |
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.
이 테스트는 어떤 테스트 얘기한거야? 제대로 변환되는지 테스트?
간단한거면 여기다 fun main() {} 만들고 돌려볼 수 있어
…plete-mappers # Conflicts: # core/model/src/main/java/com/wafflestudio/snutt2/core/model/data/lecture/OriginalLecture.kt
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.
궁금한점) 네트워크 모델 변환 함수는 core:network 에 있어야 하는 거 아닌가? 그래서 이름도 toExternalModel 인거고.
나도 그거 찾아봤었는데 NIA 기준으로 |
우리는, core:network 의 API들은 네트워크 모델을 반환하고 core:database 는 데이터베이스 모델을 반환하는 걸로 가 보자 |
그런데 우리 core:model 아직 하나도 안 만든거 아니야? |
변환 함수를 생성하는 과정에서
예상했던 것보다 나중에 Revisit 해야 할 부분이 많아서,
네트워크 모델 변환 함수 생성/데이터베이스 모델 변환 함수 생성/확장 함수 생성 세 PR로 나누게 되었다.
이 PR에서는