Migrate package to use swift package manager - #156
jslattery26 wants to merge 8 commits into
Conversation
philips77
left a comment
philips77
left a comment
There was a problem hiding this comment.
Hi, thanks for the PR!
Why did you move all the darvin/Classes code to mcumgr_flutter/Sources/mcumgr_flutter? Perhaps we could get rid of one of these mcumgr_fluitter?
I also did some comments to the code.
| kotlinOut: 'android/src/main/kotlin/no/nordicsemi/android/mcumgr_flutter/Messages.g.kt', | ||
| kotlinOptions: KotlinOptions(package: "no.nordicsemi.android.mcumgr_flutter"), | ||
| swiftOut: 'darwin/Classes/Messages.g.swift', | ||
| swiftOut: 'ios/mcumgr_flutter/Sources/mcumgr_flutter/messages.g.swift', |
There was a problem hiding this comment.
This should be darvin, not ios?
There was a problem hiding this comment.
Nice catch, i didn't change it from the migration step.
| int? opOverride, | ||
| int? flagsOverride, | ||
| ); | ||
| void setupDecorator(String remoteId, Uint8List? suffix, int? opOverride, int? flagsOverride); |
There was a problem hiding this comment.
Isn't it more readable in multiple lines? Below as well.
| self.logCategory = category | ||
| self.logLevel = level | ||
| self.logDateTime = Int64(timeInterval * 1000) | ||
| logCategory = category |
There was a problem hiding this comment.
These self were here to keep the style the same. It needs to be in message, but following properties look better if they are also prefixed like that.
There was a problem hiding this comment.
Oops, I think this is my swift formatting, i'll have to fix these ones
| return .default | ||
| case .shell: return .shell | ||
| case .suit: return .suit | ||
| case .memfault: return .default |
There was a problem hiding this comment.
Perhaps a new category instead of defaulting? The default is not a default category... it's just an unfortunate name for the "OS Manager" which historically been named Default Manager.
|
The reason a lot of files were moved was because I followed this migration guide, and this might answer some of your other questions: |
|
@philips77 I think I resolved all of the issues. Let me know if theres anything else i can do. I appreciate the in-depth review |
|
@philips77 ping :) (We're in August, already) |
|
PSA: I believe @philips77 wanted to do some thorough testing before merging into main. Unfortunately he's on much needed vacation at the moment. We will resume work on late August / early September. Sorry to everyone waiting for this for the delay. You're welcome to provide additional feedback if you consider this branch to be in good shape to merge. |
|
Hi @dinesharjani @philips77 any updates on this? |
4 participants
still backwards compatible with cocoapods