-
Notifications
You must be signed in to change notification settings - Fork 3
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 usecase to set external camera ip [#55] #57
Conversation
기본적으로 #52 에서 만들어진 변경을 참고하고, #54 의 최신 entity관련 변경사항도 반영하여 작성했습니다. 현 시점 PR은 빌드가 성공하는 수준에서 마무리 된 상태입니다. 실제 동작을 HomeScreen에 연결된 ViewModel에서 대략적으로 테스트 하려했지만, 안드로이드에 대한 이해가 짧아 아래같은 에러를 만나고 포기했습니다.
정상 동작 확인할 수 있는 방법 조언해주시면 감사하겠습니다! |
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.
String 하나만 박아도 괜찮을까 싶긴 했지만, 다른걸 뭘 넣어야할지 아직 불확실해서 이대로 두었습니다.
예를들면 정상 String검증, Uint32타입으로의 변환, Port필드 추가 등의 아이디어는 있었지만, 현시점 구현에서 불필요할 것이라고도 생각되서요...
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.
저는 일단 이대로 가는게 어떨지 싶습니다 ㅎㅎ
정상 String검증이 아마 ip 타입 형태의 string 포맷인지 아닌지 검사 여부를 의미하는 것으로 이해했는데요,
필요한 기능이긴 하나 해당 코드위치에 대한 고민이 필요할 것 같고 ExternalCameraIP.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.
ExternalCameraIP를 지금 당장 지우고 raw String으로 바꾸는 것도 잠깐 고민했는데, 혹시 모를 확장성을 위해 남겨두도록 하겠습니다! 나중에 잘 판단해서 작업 부탁드립니다ㅎㅎ
.build() | ||
} | ||
} | ||
|
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.
특히 이부분 동작을 어떻게 확인해야하는지 모르겠습니다... 프로토 버프 잘 저장되려나요?ㅋㅋㅋ @DokySp
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.
대신 검증해주셔서 감사합니다!
Hilt 의존성 주입 에러로 보입니다. domain 모듈 안에 UseCaseModule에 아래 코드 추가하면 미주입 에러가 해결되는 것으로 확인됩니다.
|
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.
처음인데 고생많았네요 ㅠ
아래 코멘트 조금 달아두었습니다~
appPreferencesFlow.map { | ||
val address = it.externalCameraIp | ||
if (address.isNullOrEmpty()) { | ||
ExternalCameraIP("0.0.0.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.
유효하지 않은 값일 때, 정해진 초기값을 repository에서 내려주는 것보다는 AppPreferencesSerializer.kt에서 초기값을 지정해주는 것이 좋을 것 같습니다 😄
class AppPreferencesSerializer @Inject constructor(): Serializer<AppPreferences> {
override val defaultValue: AppPreferences
get() = AppPreferences.newBuilder().run {
// Add proto datastore default value here.
// You need to check default value of each types from link below
// https://protobuf.dev/programming-guides/proto3/
// ex> isDebugMode = true
externalCameraIp = AppPolicy.DEFAULT_EXTERNAL_CAMERA_IP
build()
}
util
모듈에 위와같이 기본값 혹은 정책 관련 내용들은 Policy.kt 파일로 만들어 관리하면 추후 개발할 때 유용할 것 같습니다 ㅎㅎ
object AppPolicy {
const val DEFAULT_EXTERNAL_CAMERA_IP = "0.0.0.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.
제안 감사합니다! AppPreferencesSerializer의 constructor가 그런 용도로 쓰일 수 있다는걸 몰랐네요 :)
혹시 새 util의 이름이 AppPolicy.kt
가 아니라 Policy.kt
인 이유가 있나요? 예를들어 AppPolicy가 아니라 다른 Policy도 함께 들어가는 상황을 염두에 두기위해?
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.
슬랙으로 의논한것처럼 AppPolicy.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.
저는 일단 이대로 가는게 어떨지 싶습니다 ㅎㅎ
정상 String검증이 아마 ip 타입 형태의 string 포맷인지 아닌지 검사 여부를 의미하는 것으로 이해했는데요,
필요한 기능이긴 하나 해당 코드위치에 대한 고민이 필요할 것 같고 ExternalCameraIP.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.
고생하셨습니다 👍👍
리뷰 감사합니다! 머지하겠습니다 :) |
[Issue] #55
[Descriptions]
ExternalCameraIP
entity with single address String