Skip to content

fix(cli): regression on --config not accepting file paths#8783

Merged
amrbashir merged 4 commits intodevfrom
fix/config-arg
Feb 7, 2024
Merged

fix(cli): regression on --config not accepting file paths#8783
amrbashir merged 4 commits intodevfrom
fix/config-arg

Conversation

@lucasfernog
Copy link
Copy Markdown
Member

No description provided.

@lucasfernog lucasfernog requested a review from a team as a code owner February 5, 2024 17:30
Comment thread tooling/cli/src/build.rs Outdated
options.config = merge_config;

let config = get_config(target, options.config.as_deref())?;
let config = get_config(target, options.config.as_ref().map(|c| &c.0))?;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can't we just config as an argument to the setup function?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

we do not do this overall in the codebase, get_config is cached so it's cheap if it's already been resolved, otherwise we need to pass the config around all codebase

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yeah, but config is parsed right before calling setup so it is better to just pass the config to it, than getting from cache again

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

you're right it wasn't that complicated in the end.. thanks

Comment thread tooling/cli/src/dev.rs Outdated
Comment thread tooling/cli/src/dev.rs Outdated
Comment thread tooling/cli/src/mobile/mod.rs Outdated
Comment thread tooling/cli/src/mobile/mod.rs Outdated
@lucasfernog
Copy link
Copy Markdown
Member Author

stupid android test CI still broken..

@lucasfernog lucasfernog requested a review from amrbashir February 6, 2024 10:20
@amrbashir amrbashir merged commit fb0d997 into dev Feb 7, 2024
@amrbashir amrbashir deleted the fix/config-arg branch February 7, 2024 15:08
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