Fix CI for Swift Package Manager #33
Conversation
Co-authored-by: Giordano Menegazzi <[email protected]>
…n-ios into feature/spm-fix-ci # Conflicts: # Examples/Swift/ViewController.swift
Patrick-Kladek
changed the title
MapboxNavigationTests/Sources/Tests/NavigationViewControllerTests.swift
Outdated
Show resolved
Hide resolved
|
@StephanPartzsch thank you for the review. I've noticed a lot of code style issues and wanted to ask if you use a code formatter? I started using SwiftFormat, configured it to reformat on commit automatically and it worked flawlessly for over 2 months now. Would you accept this change, if I add it? I would let it configure itself from the current code style at first, and then tweak the settings if needed. |
|
At the moment there is no formatter configuration in the codebase, but it is a good idea to add one. This helps us having a consistent code style. |
|
I've also noticed there is a As adding these tools will introduce a huge amount of changes, I'll create a new PR from this branch, add SwiftFormat, also enable SwiftLint, and fix all the errors/warnings found by the tools. Once we can ensure a consistent code style we can go ahead and finalize this PR. |
|
Yes I agree, let's do reformatting of the whole codebase in a separate PR. IMHO anything that we can automate and follows more or less the default XCode formatting is fine by me. Let's do one big reformatting PR and push this through to avoid too many conflicts. |
|
Having two separate PR is fine. |
|
@StephanPartzsch I guess with identiation style you meant to use spaces instead of tabs. I've fixed all issues you pointed out, please review again. In the meantime, I'll continue on the code formatting PR so issues like this are a thing of the past. |
|
Hi everyone, I've prepared another PR https://github.com/Patrick-Kladek/maplibre-navigation-ios/pull/2 to ensure consistent code style by auto formatting. Can you please take a first look at the style so we can discuss styling there further. I'm not able to create a PR in repository as its base branch is in my Repo, I can only do that after this PR is merged. |
|
LGTM |
|
@boldtrn can you please review it as well and merge it |
|
I think it is very unfortunate that we are loosing the examples with this change, maybe we can setup a second repo with the examples or something like this as a followup? |
| @@ -197,7 +197,7 @@ public class CarPlayNavigationViewController: UIViewController, MLNMapViewDelega | |||
| let location = notification.userInfo![RouteControllerNotificationUserInfoKey.locationKey] as! CLLocation | |||
|
|
|||
| // Update the user puck | |||
| let camera = MLNMapCamera(lookingAtCenter: location.coordinate, fromDistance: 120, pitch: 60, heading: location.course) | |||
| let camera = MLNMapCamera(lookingAtCenter: location.coordinate, acrossDistance: 120, pitch: 60, heading: location.course) | |||
There was a problem hiding this comment.
While I don't mind this change, it is a bit weird, that this is here. Is this intentional? Why is this changed? This seems to be unrelated to SPM?
There was a problem hiding this comment.
This is the warning I got and thats why I changed it:
'init(lookingAtCenter:fromDistance:pitch:heading:)' is deprecated: Use -cameraLookingAtCenterCoordinate:acrossDistance:pitch:heading: or -cameraLookingAtCenterCoordinate:altitude:pitch:heading:.
| arrow.lineWidth = NSExpression(forMLNInterpolating: .zoomLevelVariable, | ||
| curveType: .linear, | ||
| parameters: nil, | ||
| stops: NSExpression(forConstantValue: MLNRouteLineWidthByZoomLevel.multiplied(by: 0.7))) |
There was a problem hiding this comment.
Ah ok, this is about #6?
If it works, I am fine with it, but in general I think we should try to limit PRs to one issue/topic as this is hidden between the SPM changes.
There was a problem hiding this comment.
Yes this is related to the errors logged to console. I don't feel comfortable releasing an app to our customers when I see errors. I could verify after the changes these errors are not printed anymore and navigation seems to work fine
There was a problem hiding this comment.
Thanks a lot for working on this PR and the effort that went into it, also from @MaartenZonneveld 👍
I am not a fan of the hidden changes here and there. It's nothing major and if you made sure they work, I am fine with merging this as it is. I guess there might be some other issues hidden that we only find in integration testing later on, so let's merge this and fix stuff that might be wrong further down the line.
I rather fix small issues right away instead of opening up a discussion, and then spending 90% of time managing a one line change, that then drags on for days. Retroactively speaking this could be the wrong approach here, so thanks for catching that. I'll try to limit the scope of my changes per PR from now on. Do you want me to revert the NSExpression change and submit it in a new PR? |
Fixing a few small bugs in PR is fine I guess, but fixing a small bug in 300 file change PR that is about something specific, doesn't feel right. If something breaks I tend to look at the commit history and look at the commits that sound related first.
Just for the future, if you verified that it works as expected, let's go ahead 👍 |
Creating a PR here in hopes the CI will run.
This PR was done to finalize the work from #24. As the original Author is not responding we continue the official discussion here.