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

Convert double-quoted includes to angle includes #6537

Merged
merged 21 commits into from
Sep 24, 2024

Conversation

tomekzaw
Copy link
Member

@tomekzaw tomekzaw commented Sep 23, 2024

Summary

This PR:

  • Changes all #include "someheader.h to #include <reanimated/path/to/someheader.h>
    • This is to avoid potential conflicts with headers from other dependencies
  • Updates build scripts (CMakeLists.txt and RNReanimated.podspec) to change header paths
    • On iOS, we introduce subsubspecs, one for each folder inside Common/cpp/reanimated or Common/cpp/worklets, as well as one for apple/reanimated, so that header paths are correct (otherwise CocoaPods flattens nested directories)
  • Updates CI workflows (mostly yarn lint:{android,apple,common})
  • Removes unused includes wherever possible
-#include "AnimatedSensorModule.h"
+#include <reanimated/AnimatedSensor/AnimatedSensorModule.h>

Next PRs:

  • Convert #import <RNReanimated/*.h> to #import <reanimated/**/*.h> in apple/ directory
  • Rename reanimated C++ namespace to swmansion::reanimated and swmansion::worklets
  • Move disallow DEBUG workflow step to a separate script
  • Remove empty.cpp

Test plan

@tomekzaw tomekzaw requested a review from tjzel September 23, 2024 16:10
@tjzel
Copy link
Collaborator

tjzel commented Sep 23, 2024

Plz fix import orders for CI 🙏

@tomekzaw tomekzaw requested a review from piaskowyk September 23, 2024 21:25
@tomekzaw tomekzaw marked this pull request as ready for review September 23, 2024 21:27
@tomekzaw tomekzaw changed the title Convert double-quoted imports to angle imports Convert double-quoted includes to angle includes Sep 23, 2024
@tomekzaw tomekzaw requested a review from kmagiera as a code owner September 23, 2024 21:33
Copy link
Collaborator

@tjzel tjzel left a comment

Choose a reason for hiding this comment

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

All good, I mostly addressed some inconsistencies but feel free to address them as you like.

Personally I'm not a fan of separating includes with newlines but since we don't have any tools for that I guess it's fine.

Copy link
Member

@piaskowyk piaskowyk left a comment

Choose a reason for hiding this comment

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

👍

@tomekzaw tomekzaw requested a review from tjzel September 24, 2024 10:07
@tomekzaw tomekzaw added this pull request to the merge queue Sep 24, 2024
Copy link
Collaborator

@tjzel tjzel left a comment

Choose a reason for hiding this comment

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

Bear Lets Merge

Merged via the queue into main with commit 5865d46 Sep 24, 2024
17 checks passed
@tomekzaw tomekzaw deleted the @tomekzaw/angle-bracket-imports branch September 24, 2024 10:39
github-merge-queue bot pushed a commit that referenced this pull request Nov 18, 2024
## Summary
PR adding back headers removed in
#6537.
For some reason, locally they are not needed, probably to some different
setup or having the libraries set globally somewhere, but it caused
failures in Expensify:
https://github.com/Expensify/App/actions/runs/11857735322/job/33046828856.
They should be included since methods from them are used.
## Test plan
Not really, just run CIs probably.
tjzel pushed a commit that referenced this pull request Nov 20, 2024
## Summary
PR adding back headers removed in
#6537.
For some reason, locally they are not needed, probably to some different
setup or having the libraries set globally somewhere, but it caused
failures in Expensify:
https://github.com/Expensify/App/actions/runs/11857735322/job/33046828856.
They should be included since methods from them are used.
## Test plan
Not really, just run CIs probably.
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.

3 participants