Workspace awareness #47
Conversation
|
Any comments? |
src/bundle/settings.rs
Outdated
| package_type: Option<PackageType>, // If `None`, use the default package type for this os | ||
| package: PackageSettings, | ||
| package_type: Option<PackageType>, | ||
| // If `None`, use the default package type for this os |
There was a problem hiding this comment.
It's a little confusing to have this comment here, since it looks like it applies to the target field. If it won't fit on the end of the package_type line, then could we move it to just above that line?
There was a problem hiding this comment.
Hmm... seems like it's the IntelliJ automatic formatting that pushed it to a new line
(a new line that doesn't make sense IMO).... so will put it back manually.
src/bundle/settings.rs
Outdated
| /* | ||
| Try to load a set of CargoSettings from a "Cargo.toml" file in the specified directory | ||
| */ | ||
| fn cargo_settings(dir: &PathBuf) -> ::Result<CargoSettings> { |
There was a problem hiding this comment.
Seems like this would make more sense as a method of CargoSettings, rather than Settings. Maybe CargoSettings::load()?
There was a problem hiding this comment.
Yes, off course! Makes sense.
src/bundle/settings.rs
Outdated
| let cargo_settings = Settings::cargo_settings(¤t_dir)?; | ||
| let package = match cargo_settings.package { | ||
| Some(package_info) => package_info, | ||
| None => return Err("No 'package' info found in 'Cargo.toml'".into()) |
There was a problem hiding this comment.
In this case, this can be written more succinctly as just:
None => bail!("No 'package' info found in 'Cargo.toml'"),
There was a problem hiding this comment.
I was getting some "note: this error originates in a macro outside of the current crate
" errors, but will try to fix them.
src/bundle/settings.rs
Outdated
| to, or do we have to walk up the file system until "/" to see if we can find a | ||
| Cargo.toml? | ||
| */ | ||
| fn get_workspace_dir(current_dir: &PathBuf) -> PathBuf { |
There was a problem hiding this comment.
Looks like this method is still WIP?
There was a problem hiding this comment.
This is where I mentioned that the comments are more on a "full" implementation, but the supplied implementation is minimal.....so I propose to strip the comments back to match the code, and add issues about future enhancements - if you agree.
|
OK, @mdsteele I addressed those comments, and rebased on master and merged your changes made since I started - take a look. |
…from that - check for Cargo.toml files between the current directory and file system root - load the first one found and assume it's the "parent" of current directory and the workspace "root" - derive the target directory (where binary will be found etc) from that, not the local directory - package into the workspace target directory. - Add CargoSettings::load function to load CargoSettings
|
Thanks for the changes! |
Here is a WIP Pull-request that implements simple workspace awareness.
Care to take a look at give me some review comments / feedback?
I have left in some comments that go beyond the scope of the actual code changes as FYI.
I would take parts of them and make them separate enhancement issues for cargo-bundle if you agree, and remove the comments - so the comments match the code.
Note that I extracted CargoSettings from Settings (and used just Package) as: