Add create-package command to CLI #38
Conversation
| @@ -37,7 +37,7 @@ impl<'a> CreatePackageConfig<'a> { | |||
| } | |||
| } | |||
|
|
|||
| #[derive(Debug, Eq, PartialEq, Hash)] | |||
| #[derive(Debug, Eq, PartialEq, Hash, Clone)] | |||
| @@ -1 +1,7 @@ | |||
| mod clap_app; | |||
There was a problem hiding this comment.
#![deny(missing_docs)] at the top of the file
|
|
||
| create_package(config); | ||
|
|
||
| println!("Package written out to {}", out_dir.display()); |
There was a problem hiding this comment.
Our commands should be silent by default. Mind removing this?
crates/swift-bridge-cli/src/main.rs
Outdated
| @@ -1 +1,5 @@ | |||
| fn main() {} | |||
| use swift_bridge_cli::app::{cli, handle_matches}; | |||
There was a problem hiding this comment.
The book chapter on Swift packages should mention the swift-bridge CLI command
i.e. with a little bit of text and an example like:
cargo install -f swift-bridge-cli
swift-bridge create-package ...
Similar to what we do with the API section of the Swift Package chapter
|
|
||
| pub mod app { |
There was a problem hiding this comment.
Can we just export a single run() function (or some other name) that the main.rs calls?
And in that function we can call handle_matches(cli().get_matches());?
| ]; | ||
| platforms.iter().for_each(|(platform, path)| { | ||
| if let Some(path) = path { | ||
| config.paths.insert(platform.clone(), path); |
There was a problem hiding this comment.
Don't need this clone() if we implement Copy
| out_dir: &out_dir, | ||
| package_name: name | ||
| }; | ||
| let platforms = vec![ |
There was a problem hiding this comment.
let platforms = ApplePlatform::ALL;
for platform in platforms {
if let Some(path) = matches.value_of(platform.as_dir_name()) {
config.paths.insert(platform, path);
}
}
// Where we define `ApplePlatform` ...
impl ApplePlatform {
pub fn as_dir_name(&self) -> &'static str { ... }
const ALL: &'static [Self] = &[ApplePlatform::Foo, ApplePlatform::Bar, ...];
}
|
Looks like the integration test suite got stuck https://github.com/chinedufn/swift-bridge/runs/5514191148?check_suite_focus=true? Any idea what happened here? |
I saw that too. I'm not sure what happened either since I did not add anything to the other packages except for a Debug derive. Maybe let's see if it happens again. I'll rebase later. |
Jomy10
force-pushed
the
cli
branch
2 times, most recently
from
1e47e0f to
e2d83f0
Compare
|
Alright, that should be everythin. |
| /// The name for the Swift package | ||
| pub package_name: &'a str, | ||
| } | ||
|
|
||
| impl<'a> CreatePackageConfig<'a> { | ||
| impl<'a, P: AsRef<Path> + ?Sized> CreatePackageConfig<'a, P> { |
There was a problem hiding this comment.
We don't want this generic bounds because it constraints every field to use the same monomorphized P
| fn handle_create_package(matches: &ArgMatches) { | ||
| let bridges_dir = matches.value_of("bridges-dir").unwrap(); // required | ||
| let out_dir = matches.value_of("out-dir").map(|p| Path::new(p)).unwrap(); // required | ||
| let name = matches.value_of("name").unwrap_or(out_dir.file_name().expect("Couldn't derive Swift Package name from output directory").to_str().unwrap()); |
There was a problem hiding this comment.
Using the name of the directory as the package name feels a bit unexpected.
Can we start by just making name a required field and if in the future it ends up feeling redundant we can consider a shortcut like this?
|
|
||
| /// Executes the `create-package` command | ||
| fn handle_create_package(matches: &ArgMatches) { | ||
| let bridges_dir = matches.value_of("bridges-dir").unwrap(); // required |
There was a problem hiding this comment.
If know that if we use the #[derive(Debug, Parser)] clap macro we should automatically get a nice help message if someone forgot a required field.
Is there an easy way to do that without using the macro?
If not, should we just use the macro? What do you think?
There was a problem hiding this comment.
Using the Builder api has the same result, so this unwrap statement should never fail as Clap will always show a helpful error message before this line ever gets executed.
| let mut paths: HashMap<ApplePlatform, Box<&Path>> = HashMap::new(); | ||
| for platform in platforms { | ||
| let path = matches.value_of(platform.dir_name()); | ||
| let path = path.map(|p| Path::new(p)); |
There was a problem hiding this comment.
We should be able to write this without all of this mapping and boxing, but no big deal for now.
Added the CLI, including the create-pakage command allowing to create a Swift Package from Rust code. The book chapter has also been updated to reflect the new changes.
|
Ok, all of your notes should be addressed. I've changed the insertion of the platform paths to the best solution I could come up with. |
|
Great work! |
I have added the cli and the
create-packagesubcommandLet me know if you think my approach is good or not.