Skip to content
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

Changed swift code and podspec to use resource bundle #4519

Closed

Conversation

Francisco-Costa98
Copy link
Contributor

@Francisco-Costa98 Francisco-Costa98 commented Aug 4, 2023

Description

solves issue #4516
changed to use the non-deprecated resource_bundle in the podspec file instead of resources -> this fixes the multiple output commands produce Assets.Car error since the release of xcode 12

---------- CHECKLIST ----------

  1. Add related labels (bug, feature, new API(s), SEMVER-MAJOR, needs-backporting, etc.). - cant seem to be able to do this
  2. Update progress status on the project board.
  3. Request a review from the team, if not a draft.
  4. Add targeted milestone, when applicable.
  5. Create ticket tracking addition of public documentation pages entry, when applicable.
  6. Update Changelog.
  7. Rebase onto main from the branch before merge.
    -->

* vk/NAVIOS-41-time-diff-callouts: fixed method for picking time diff callout coordinate; CHANGELOG updated
* vk/NAVIOS-908-cp-templates-fix: added completions to setting and popping to root CP templates; CHANGELOG updated
* vk/NAVIOS-1233-carplay-maneuver-color: fixed applying correct style type when generating maneuver arrow for carplay; CHANGELOG updated
….0 (mapbox#4483) (mapbox#4485)

* [Snapshot] Update dependencies: Common SDK to 23.6.0, Maps 10.14.0, NN to 137.0.
Co-authored-by: Nastassia Makaranka <[email protected]>
@Francisco-Costa98 Francisco-Costa98 marked this pull request as ready for review August 4, 2023 16:06
@Francisco-Costa98 Francisco-Costa98 requested a review from a team as a code owner August 4, 2023 16:06
Copy link
Contributor

@Udumft Udumft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, @Francisco-Costa98! Thanks for contributing! I see few NITs, but otherwise it should be good to go. And also, please retarget this PR to main branch instead. v2.14 is not planned to have patches, so this fix won't be available if merged there. If you are using 2.14 it is still recommended to update to the latest, there should be no effort on your side to do so.

Sources/MapboxNavigation/Bundle.swift Outdated Show resolved Hide resolved
Comment on lines 16 to 23
guard let resourceBundleURL = frameworkBundle.url(
forResource: "MapboxNavigationResources", withExtension: "bundle")
else { fatalError("MapboxNavigationResources.bundle not found!") }

guard let resourceBundle = Bundle(url: resourceBundleURL)
else { fatalError("Cannot access MySDK.bundle!") }

return resourceBundle
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please correct indentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will fix - im fairly new to xcode dev, going to have a look at how to setup linting for stuff like this properly, do you have any recommendations?

@Francisco-Costa98
Copy link
Contributor Author

Hey, @Francisco-Costa98! Thanks for contributing! I see few NITs, but otherwise it should be good to go. And also, please retarget this PR to main branch instead. v2.14 is not planned to have patches, so this fix won't be available if merged there. If you are using 2.14 it is still recommended to update to the latest, there should be no effort on your side to do so.

Thank you for the feedback - ill open it against main - although currently im locked into this specific version for my project due to also depending on the react native mapbox library, going try to see if i can upgrade that to the latest aswell with no issues

@Francisco-Costa98 Francisco-Costa98 changed the base branch from release-v2.14 to main August 7, 2023 16:22
@Francisco-Costa98
Copy link
Contributor Author

Hey @Udumft - I created a new PR (#4525 ) here as changing to main gave me alot of changes due to making them on the version 2.14 of the branch, let me know if this is OK - closing this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants