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

Add ability to disable cross import of modules #35

Merged
merged 4 commits into from May 12, 2023

Conversation

jdanthinne
Copy link
Contributor

@jdanthinne jdanthinne commented May 11, 2023

This PR adds to ability to remove the import ArkanaKeysInterfaces statement in ArkanaKeys module when used with Cocoapods.

As I'm building an SDK using Arkana, importing via Pods, so I'm importing the Sources folder directly and this import statement was causing a compilation error, because all Sources are in the same module.

Copy link
Owner

@rogerluan rogerluan left a comment

Choose a reason for hiding this comment

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

Hi @jdanthinne 👋
Thanks for this proposal!

I left a few comments for you. Also, could you write unit tests that show that this config flag is working as expected? 🙏

template.yml Outdated Show resolved Hide resolved
lib/arkana/templates/arkana.swift.erb Outdated Show resolved Hide resolved
@jdanthinne
Copy link
Contributor Author

For the unit test, as it is a preprocessor thing, it is hard to write that in the Swift tests. I guess it should be a Ruby unit test, but I must admit I don't know how it's done (I'm mainly an iOS dev).

Copy link
Owner

@rogerluan rogerluan left a comment

Choose a reason for hiding this comment

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

💪 Thanks for addressing the changes! This LGTM 🚀

@rogerluan rogerluan merged commit 91f480b into rogerluan:main May 12, 2023
13 checks passed
@jdanthinne
Copy link
Contributor Author

Please don't make a release with this change too soon. I think I've come across a strange case where this doesn't work as planed.

@rogerluan
Copy link
Owner

😬 got it!
Thank you for the heads up. What is the issue?

@rogerluan
Copy link
Owner

@jdanthinne any update? Otherwise I'll revert this PR 🙏 please let me know.

@jdanthinne
Copy link
Contributor Author

Sorry for the delay. It seems that in some configuration, my conditional compilation doesn't work as expected. I'll try to get back to you asap.

@jdanthinne
Copy link
Contributor Author

Nevermind, it seems to work fine, finally. I guess Xcode was lost when I was switching back and forth when I was changing the code between Cocopoads and SPM usage. I re-tried everything today, and it's ok.

@rogerluan
Copy link
Owner

Sounds good. Thank you @jdanthinne 👍

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.

None yet

2 participants