Context struct proof of concept #122
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…sent Cargo.toml contents. This build will fail.
…rting a CargoManifesto -into- an NpmPackage. This means that a mutable reference to `self` is not needed. This should help with adding a lazy static variable to store the contents of the Cargo.toml file.
…compile. There is some remaining work to be done, scopes and package names are not being identified correctly. Seems hard to test the static variable, and to provide arguments once this is moved into a lazy static.
…should be public.
… a tuple. (Clearer that way?)
good idea yet, we're trying things here (for science).
…need to be moved? They might need a way to mutably access the manifest field of the manifest, if this will not be loaded until needed.
…t is to introduce a runtime context struct, that will store information needed by the command helper functions, and pass these accordingly. This saves effort regarding reading files like Cargo.toml or package.json more than once, and should help with extensibility in the future. There is still some more work to do on this (files are still read more than once, and there are some ugly borrowing things), but this does pass all tests and is a nice start on the idea :)
found in the integration tests directory, and adds a derived Deserialization trait to the structs. The NpmPackage is now public, and has methods to get the values of private members, i.e. package name, files, main, repository url, and so forth. NOTE: This may or may not be a good idea, depending on the testing strategy, as well as opinions about what should/should-not be public.
a command as a constructor parameter, this parameter is now given to the run method. This cleaned up some of the ugly borrowing stuff, and should also make it possible to reuse the same context for multiple things, if that was ever desired.
…, only the Command object is passed.
…d this into a submodule directory. This commit does not change any of the contents of the file, the next step will be to work on dividing the file into separate components.
…ml and package.json files, for Cargo and npm respectively.
…out into some separate files. Some of these might make sense being placed into the context struct, and it might be nice to have the commands in separate files.
…and functions do not need to worry about unwrapping and setting the default path themselves.
…ccept a &str parameter rather than a String.
…n moved into this directory. The reasoning behind this is that the init command seems to be the only command that uses these functions, and this might also make a 'build' command easy to implement going forward.
… refine, but this should get us much closer to only needing to read the cargo file once. If we are lucky, we might even be able to perform some other verification ahead of time, like checking whether or not the Cargo.toml file exists.
…ions, working with the progress bar, and other stuff nice and delineated from the actual act of packing, publishing, etc. This is still a work in progress, one other thing I am still thinking about is whether some of these might need to be inline, rather than working as helper functions.
… this makes more sense, and keeps some destructuring logic out of the main function.
…e work required to migrate logic into the context struct. Progress bar messages are now set in the context's wrapper functions, which call down into separate functions responsible for the actual implementation of an action. This commit will not compile if `cargo test` is run, there is still refactoring work to do to apply these changes to the test bench.
…m the manifest file.
…thing if we pursue this route.
…n epiphany. A separate file for each of the commands could make testing the actual -logic- easy, while the context will keep most of this fairlty encapsulated. There might be some nuance to this, and not every step might end up being able to be isolated out like this, but this should make a lot of the testing fairly straightforward. :) Plus, having a file for each command will make things easy to navigate!
…e refactored version of the project :)
… is not quite final, but I think this might be workable. I am expecting that the file creation, package.json info, and file writing might need to be performed in separate steps.
…e if this PoC is worth cleaning up further.
|
For context of the conversation we had: lazy_static would read the env variables like so use std::env::args;
// skip program name and subcommand
for arg in args().skip(2) {
// init code here
}Where metadata can be used to see if the value is a path, if it is open up the file and read it in. If not try for each arg. If none of them match try Doable but a bit hackish. I'm going to look through the Context stuff but this is a common pattern in Rust and used in things like cargo/rustc so I'm not opposed to this idea either, but this is a huuuuuuuge refactor so I'm inclined to go with the hack for now then deffering this to a new issue and doing piecemeal updates to switch over to context with multiple PRs that are easier to review and test. |
|
"I'm inclined to go with the hack for now then deffering this to a new issue and doing piecemeal updates to switch over to context with multiple PRs that are easier to review and test." @mgattozzi This sounds like a good plan to me too, pardon the amount of change here! I'll close this now, since it was mostly intended as a PoC :) |
|
@data-pup I'm gonna open up an issue for this so we can discuss this further :D |
|
Sounds good to me! ~ [^.^] ~ |
|
@mgattozzi Just wanted to mention that I can work on a branch that follows the env variable method you showed above, that shouldn't take too long. I'll get that done and open a (much smaller, lol) PR soon :D |
|
Awesome I'll keep an eye out for it :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Per a previous conversation in irc, I looked into what adding a context struct would look like, if we wanted to pursue that idea further. This would fix #25, and only require that the
Cargo.tomlfile be read once.This would offer some benefits, such as not needing lazy_static for the progressbar, or for the manifest. The other nice thing is that the logic for running commands can be separated from the progressbar stuff, but that's a subjective benefit I suppose. This also means commands like
initwould work whether or not the path argument is given before or after the scope argument, I believe.The downside, and I think it may be considerable, is that I think it would end up being a pretty drastic change to the existing codebase. If we don't mind requiring that the path is the last argument given to the program,
lazy_staticwould work quite fine. In that case, I think I can open a PR using the approach @mgattozzi mentioned, as that would be a simpler/smaller change to make.Feel free to tag this as blocked. This was definitely speculative work to see what that approach would look like, and I figured I would put it here for the sake of helping y'all evaluate whether that would be a desirable road to go down. :)
By the way, I don't mind rebasing all of this if needed, I've left all the dev commits for now in case you'd like to build off of a specific commit etc.
Make sure these boxes are checked! 📦✅
rustfmtinstalled and have yourcloned directory set to nightly
$ rustup override set nightly $ rustup component add rustfmt-preview --toolchain nightlyrustfmton the code base before submitting✨✨ 😄 Thanks so much for contributing to wasm-pack! 😄 ✨✨