search

maplibre / maplibre-navigation-ios

MapLibre Navigation SDK for iOS
Other
36 stars 29 forks source link

Fix CI for Swift Package Manager #33

Closed Patrick-Kladek closed 5 months ago

Patrick-Kladek commented 6 months ago

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.

Patrick-Kladek commented 5 months ago

@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.

StephanPartzsch commented 5 months ago

@Patrick-Kladek

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. Please use the indention level as it is used in the majority of files of this codebase.

Patrick-Kladek commented 5 months ago

I've also noticed there is a .swiftlint file in the repo, however, we are currently not using it.

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.

boldtrn commented 5 months ago

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.

StephanPartzsch commented 5 months ago

Having two separate PR is fine. We can finalize this one and add SwiftLint and swiftformat in a later PR as it will change the indention issues of this PR anyways. Or you can do it by hand for now @Patrick-Kladek . The number of issues wasn't that huge ;)

Patrick-Kladek commented 5 months ago

@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.

Patrick-Kladek commented 5 months ago

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.

StephanPartzsch commented 5 months ago

LGTM

Patrick-Kladek commented 5 months ago

@boldtrn can you please review it as well and merge it

boldtrn commented 5 months ago

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?

Patrick-Kladek commented 5 months ago

I am not a fan of the hidden changes here and there.

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?

boldtrn commented 5 months ago

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.

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.

Do you want me to revert the NSExpression change and submit it in a new PR?

Just for the future, if you verified that it works as expected, let's go ahead 👍