-
Notifications
You must be signed in to change notification settings - Fork 8
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
Handle non-standard codegen output dirs #165
Handle non-standard codegen output dirs #165
Conversation
This currently still fails for Android.
I haven't yet found out where the |
One possible source I found is in
|
ddefa62
to
b0f5ea3
Compare
b0f5ea3
to
16681fc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice, thank you.
I have left a couple of nits, which you can ignore or not.
Also: the checks don't seem to have run on this PR. Is that something I missed?
crates/ubrn_cli/src/config/npm.rs
Outdated
.output_dir | ||
.ios | ||
.clone() | ||
.unwrap_or("ios/generated".to_string()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the only thing I'd suggest, and it is the smallest of nits, is to default this in RnOutputDirCodegenConfig
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it might take a good deal more boiler plate, so I wouldn't be upset if you ignored this suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I've addressed this now.
Yes, you're right. They only run when merging to Let me actually merge this PR into the other one and we can continue the discussion there and see the CI jobs actually running. |
Fixes: #161
This includes the changes from #163 but is currently based againstmain
so that the workflows run.This merges into #163 which should then become green and can be merged to
main
.