feat: factor out fs calls into axoasset #229
Conversation
| @@ -48,6 +44,7 @@ parse-changelog = { version = "0.5.3", default-features = false } | |||
| newline-converter = "0.2.2" | |||
| axoproject = { version = "0.3.0", default-features = false, features = ["cargo-projects"] } | |||
| dialoguer = "0.10.4" | |||
| axoasset = "0.2.0" | |||
There was a problem hiding this comment.
In my ideal world we have some pretty fine-grained feature flags on axoasset to toggle the various compression formats on, to avoid all our projects getting bloated with unused dependencies.
There was a problem hiding this comment.
That sounds like a good idea!
cargo-dist/src/installer.rs
Outdated
| // Unwrapping here because the path is basically guaranteed to have a parent at this point | ||
| let dest_dir = installer_file.parent().unwrap(); | ||
| // Potential FIXME: Have an axoasset method that _just_ creates a file, without writing | ||
| // anything (think `File::create`) | ||
| LocalAsset::write_new("", installer_file.as_str(), dest_dir.as_str()) |
There was a problem hiding this comment.
hmm interesting, we ultimately buffer up the entire result before writing to the file anyway (to do newline cleanups), and presumably the more transactional you can make "create a file, write its contents to completion" the better, right? So maybe we should just move the file creation to the end?
Might make it harder to test this code later though...?
There was a problem hiding this comment.
Also I forget, what's the significance of making us pass the file path and the dir path?
Or wait, I think this is maybe just incorrect code, and you should be getting installer_file.filename() or something?
There was a problem hiding this comment.
It is incorrect, good catch! I'll move the write calls to the end of the function, I think when testing those functions, we might want to structure them somewhat differently anyways, so I don't think that's necessarily a huge blocker.
There was a problem hiding this comment.
As for why we pass filename and directory name separately, I have no idea, that's what the signature already looked like when I came upon it. We could probably squash those arguments, though.
|
I'm taking over responsibility for completing this PR. As far as I'm concerned it's "merged" and rebasing it is my problem. Likely won't ship for 0.1.0 but that's fine, this is ostensibly a non-functional change. |
|
Closing in favour of #295, thanks again for doing this all! (Some fs calls will remain, but they're in components that are getting rewritten anyway with #283 and axodotdev/axoproject#26) |
Culmination of the work that started with the 0.2.0 release of
axoasset, which is to remove straight fs i/o calls from cargo-dist and put them in axoasset instead. Omits the templating things for now until we have a consensus on that.Closes #221.
Needs testing, I haven't had enough time for that yet.