-
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
[feat] 환경설정 앱 버전 정보 셀 로직 추가, 강제 업데이트 기능 추가 #201 #203 #206
Conversation
스위프트 내장 Result 타입과 이름이 중복되어 변경
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.
확인했습니다.
Result 타입이 정의돼있었군요... 이런거 은근히 많이 실수하죠 ㅜ
제 생각에는 강제 업데이트 관련해서 나올 수 있는 많은 예외사항들 체크하신 것 같고,
여기서 더 나오는 예외사항들은 버그로 바로바로 수정할 수 있을 것 같아요. 멋집니다👍
마지막으로 강제 업데이트를 해야 하는 시점(최소 지원 버전)을 언제로 잡을 것인지를 잘 생각해봐야 할 것 같습니다. 치명적인 오류가 있을 때 말고는 강제 업데이트가 필요한가?에 대해 생각해봤거든요. 그래서 이 단위를 구체적으로 저희끼리 논의해보면 좋을 것 같다는 생각이 들었습니다!
어쨌든, 한 번에 많은 경우의 수를 잘 처리하신 것 같아요. 고생하셨습니다!!
// MARK: - Properties | ||
|
||
/// 업데이트 필요 여부 | ||
var needsUpdate: OptionalBool { get } |
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.
업데이트 필요 여부인데 optionalBool로 한 이유가 따로 있을까요? (아래에서 더 이어짐)
|
||
// MARK: - Inits | ||
|
||
/// 싱글턴 패턴 사용을 위해 초기화 프라이빗으로 변경 |
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.
싱글턴으로 사용하려면 초기화를 private으로 해야 하는 이유가 무엇인가요?
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.
먼저 많이들 이렇게 하시는 것 같긴 한데 상황에 따라서 필수는 아닐 수도 있을것 같긴합니다! 싱글턴은 보통 앱 전역에 걸쳐서 하나의 객체를 공유하고자 사용하고, 실제로 현재 저희의 경우 버전 매니저는 싱글턴 외의 객체를 생성할 필요가 없어서 초기화를 할 수 있도록 풀어두면 (아 이거 객체 내가 만들어서 써도 되나보다 하고) 오해할 수 있으므로 아예 프라이빗으로 변경해서 싱글턴만 가능한. 이라는 의미를 전달하면 좋지 않을까 싶어서 이렇게 해보았습니당...!
// MARK: 강제 업데이트가 발생하는 경우 *해당 버전부터 이후 모든 버전에 반드시* | ||
// 1. 릴리즈 노트 "맨 마지막"에 | ||
// 2. "버전 x.x.x" 를 포함한 안내 문구(e.g. 버전 x.x.x 이상을 유지해주새오)가 포함되어야 함 | ||
// - "버전"이랑 "x.x.x" 사이에 공백 필수 | ||
// - 이 뒤로는 버전이라는 단어 사용 금지 | ||
// "버전" 이라는 단어를 기준으로 스플릿해서 맨 마지막 문자열을 가져오고, | ||
// 해당 문자열을 "." 을 기준으로 다시 스플릿해서 파싱해서 숫자만 가져오는 방식이기 때문에 | ||
// 릴리즈 노트 맨 마지막에 2의 문구를 적어주고, 해당 문구의 "버전" 단어 뒤에 최초 숫자는 반드시 지원 버전 숫자여야 함 | ||
// 안되는 예시 (다 막줄이라고 가정) | ||
// - 버전 1.2.3 이상을 유지해주새오! 버전 업 필수!!! -> 마지막 "버전" 문자를 기준으로 파싱하기 때문에 금지 | ||
// - 버전 근데 제 생일은 2월 5일이에요 그리고 1.2.3 이상 유지해주새오! -> "버전" 다음에 쓸데없는 숫자 들어가서 안됨 |
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.
이 부분 노션이나 위키나 다른 부분에 정리해서 아예 형식으로 못박아두면 좋을 것 같아요.
특히, 버전 이라는 문자열을 기준으로 스플릿
이런 것 처럼 로직에 관련된 부분들을 코드블럭으로 같이 정리하면 더 직관적으로 확인할 수 있을 것 같습니다!
+) 그리고 추가적으로, 저희 번들 version을 매번 잘 바꿨는지 확인이 필요할 것 같아요. 이거 다 잘 해놓고 번들 버전이 안 맞으면 뭔가 validation 체크 할 때 =
이 제대로 된 결과를 안 낳을수도 있거든요(경험담). 이거는 휴먼에러니까 이 부분이랑 같이 정리해서 저희 둘 다 버전 업 할 때 체크 잘 하고, 서로 몇 번째 버전인지 PR에 명시해서 두 번 체크할 수 있도록 하면 좋을 것 같습니다!
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.
추후에 노션이나 위키에 정리해두겠습니다! 나ㅏㅏㅏ중에 여력이 생기면 정규식 공부해서 써먹으면 훨씬 낫지 않을까 하는 생각을 하긴 했는데 지금 단계에서는 역부족이라...껄껄...
버전 체크는 알려주셔서 감사해용 꼭 꼼꼼히 체크하겠습니다!
private func parseMinimumRequiredVersion() -> [Int]? { | ||
self.appStoreVersionInfo?.releaseNotes | ||
.components(separatedBy: "버전") | ||
.last? | ||
.components(separatedBy: " ") | ||
.filter { $0.first?.isNumber == true} | ||
.first? | ||
.split(separator: ".") | ||
.compactMap { Int(String($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.
좋긴 한데.. 뭔가 불안한 느낌... 버전 상태를 체크하는 조금 더 좋은 방식을 저도 같이 고민해보겠습니다!
var needsUpdate: OptionalBool { | ||
guard let installedVersion = self.installedVersion, | ||
let latestVersion = self.latestVersion | ||
else { return .nil } |
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.
위에서 쓴 것 처럼, 여기서 nil일 필요가 있나 싶긴 합니다! false로도 충분할 것 같은데 nil을 쓰신 이유가 궁금해요!
여기서 나아가서 Bool 타입이 왜 true/false로만 정의되고 다른 타입과 다르게 옵셔널이 없는건지 이유도 같이 생각해볼 수 있을 것 같아요! (하시라는 뜻 아님 정답 있다는 뜻 아님 저도 항상 왜 Bool은 옵셔널을 없게 만들었을까가 궁금했어서요)
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.
여기서 이렇게 한 이유는 저는 버전 셀의 상태를 크게 3개로 나눌 수 있다고 생각했는데요,
일단 기본적으로 업데이트 여부를 체크하려면 엔드포인트에서 정보를 잘 받아오고, 성공적으로 파싱해야 이후에 해당 정보에 따라 "업데이트가 필요합니다(true)" 혹은 "최신버전입니다(false)" 를 나타내는데, 이렇게 true/false 로만 분기하는 경우 데이터를 받아오기 전이나 받아오는 과정에서 인터넷에 연결이 안된다던가 파싱에 실패한다던가 하는 등의 문제가 발생한 케이스를 구분할 수 없다고 생각했어요!
저는 이런 경우에는 업데이트 관련 정보가 없으니까 nil 이라고 생각했고, 현재 유저가 설치한 버전을 나타내는 것이 최선이라고 생각해서 그렇게 처리했습니다!
저도 사실 왜 불리언은 옵셔널이 없을까 항상 열받...지 않고 궁금했는데 단순히 참이 아니면 거짓일 수 밖에 없어서 그렇게 하지 않았나 정도로 생각하고 말았던 것 같아요...은빈님 생각도 궁금합니다...암튼 근데 세상은 단순히 이분법적으로 나눌 수 없으니까!!!!! 개인적으로는 이번 플젝뿐만이 아니라 종종 코드를 작성할 때 초기화 전이라던가 이런 경우에 단순히 참/거짓 외에 nil 상태가 더 필요하다고 느꼈습니당(저만 그랬을수도,,..ㅎ) 예ㅔㅔㅔ전에 cs193p 들을 때 과제할 때 옵셔널 불리언 같은 자료형이 필요한 경우가 생길 수 있는데 그러면 함 직접 선언해봐라~ 그런식으로 얘기하셨던 거 같아서 함 해봤어요ㅋㅋㅋ
저도 약간 짜면서 버전 2는 몰라도 버전 1에서 강제 업데이트 할 일이 생길까 싶긴 했는데 (버전 2에서는 강제 업데이트 이제 서버 api 사용하면 이렇게 더러운 로직 아닐테니까..) 또 계속 유지보수하다 보면 미래는 알 수 없으니까 흑흑 일단 추가했습니다...맞아요 강제 업데이트 단위에 대해서는 얘기해봐야 될 거 같은데 저도 마찬가지로 사실 치명적인 버그 외에 필요한가 싶네용..! 짜면서 아 이거 이렇게 하는 게 맞나 싶었는데 일단 무엇보다 제가 번아웃 상태라 귀찮았고...(ㅋㅋㅋ) 또 은빈님도 바쁘실 것 같아서 아 질문 받아주시기엔 바쁘실듯 하는 핑계로 그냥 얼렁뚱땅 짰는데,,, 앞으로는 잘 모르겠으면 일단 질문해야겠다 하는 반성을 했읍니다(함께 고민하면 족금 더 생산적이니까...)...앞으론 물음표 살인마가 되어버릴수도 있지만 천천히 대답해주셔도 됩니다... 그리고 제가 1.1.x 웬만큼 마무리 하면 배포 자동화에 관해서 좀 찾아보려고 하는데 그때 번들 버전업 관련해서 휴먼 에러 줄일 수 있는 방법도 함 보고 정리해보겠습니당..! |
반영 내용
강제 업데이트 로직을 추가했습니다.
환경설정 버전 정보 셀에 로직을 추가했습니다.
지난 번 문제 사항...