Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix (and rewrite) dx init/dx new #2822

Merged
merged 15 commits into from
Nov 16, 2024

Conversation

Andrew15-5
Copy link
Contributor

@Andrew15-5 Andrew15-5 commented Aug 12, 2024

Fixes issue in #2788 (comment). I have found few semantic breakages and bugs, but upon trying to fix stuff and dig deeper to get the whole image of what the current state of init/new subcommands is trying to do, I stumbled upon even more bugs and performance issues (calling MetadataCommand from incorrect CWD causes a 2 s slowdown!). Here is my monologue with a little bit more context.

Here I am cooking a new version of those subcommands. The workspace change will be mostly untouched as I'm not familiar with it, and it doesn't overlap with the problem at hand.

The first commit contains a ton of commented code, which most likely breaks the new command (at least in some way). But I fully rewrote (refactored + added a few things) the init command. Had to get an absolute path without resolving symbolic links, so I used a path-absolutize crate (for free). Second commit adds one dev dependency for running and testing commands (including dx), it's pretty small. I rarely wrote tests before, but these once look very nice. I think I balanced abstraction and clarity pretty well.

One thought I now have (since my testing strategy has changed from the beginning) is to put init subcommmand tests to its own file instead of the main one.

Notes (to not forget):

  • Currently no way to use non-default branch (tag/revision) when using a cargo-generate template. Need to add/copy branch, tag, revision options from cargo-generate. Should this be a separate PR or it can be included here? It by itself probably will be very small.

Temporary messed up new subcommand.

BREAKING CHANGE: changed order and names of CLI arguments/options
Added a note describing the purpose of those tests as I don't think
their names are clear enough. And I don't want to create test with a
paragraph in its name.
@Andrew15-5
Copy link
Contributor Author

I also added the branch option to be able to use (unmerged) branches from a remote template (git) repository (#2788 (comment)). As a complete set, also added revision and tag options. This will open a possibility for a wider range of reproducible projects/bugs.

@Andrew15-5
Copy link
Contributor Author

Current implementation of dx init and dx new follows the behavior of cargo init and cargo new respectively. Specifically, how path and name work. This is technically a breaking change, but for most use cases almost nothing has changed. IMO, the most noticeable change is that path is required for dx new. I laid out the problem here.

@Andrew15-5 Andrew15-5 marked this pull request as ready for review August 21, 2024 22:52
@Andrew15-5
Copy link
Contributor Author

Andrew15-5 commented Aug 21, 2024

Another notes:

  • vcs not only practically was set to None in both branches of the condition, but it also was loosely dependent on metadata which feels like not a good thing to do. Moreover, cargo-generate won't create a VCS if already inside VCS. And lastly, if anything, we can have a --vcs option to use it directly as intended.
  • the mutual exclusivity of some options is enforced by clap, so I'm not sure if tests for this should be written, but my gut feeling says that they should be, just in case.
  • here I'm pretty confident that I set a correct current directory for metadata command, but not on 100%.
  • I could've compressed the tests more by substituting repetitive code with a macro, so that it will show panic in the correct place (2 places), but it seems a bit less readable, including more verbose logs.
  • maybe use a local (in-repo) micro-template for the tests? This will remove the necessity for Internet connection and that you must pass the --subtemplate option until Default sub template is not automatically used in silent mode (without user interaction) cargo-generate/cargo-generate#1181 is fixed. Reducing things that can go wrong is always better.
  • maybe unpin cargo-generate's patch version so that we can automatically get the above issue fixed as soon as it will be available?

@Andrew15-5 Andrew15-5 marked this pull request as ready for review November 12, 2024 16:39
@Andrew15-5
Copy link
Contributor Author

I found that Error::CargoMetadata variant is gone, so I recreated it in the place where it used before it was removed.

I was surprised that the tests failed after I initially thought that I'm done. But apparently I was not the first one: assert-rs/assert_cmd#131. So because of this limitation I used escargot instead of assert_cmd. I did use the new (fancy) LazyLock for static one-time cargo build, which is expensive to call.

The tests still pass without any changes, so that's cool. :)

Do we need some sort of "workspace" dx new/dx init tests?

Also, I updated the Cargo.lock file, which is now has some crates with newer versions. Is it fine, or should I revert them?

@Andrew15-5
Copy link
Contributor Author

Oh no... It's still not in the stable version (for some platforms?).
https://github.com/DioxusLabs/dioxus/actions/runs/11802634307/job/32878728014?pr=2822

@Andrew15-5
Copy link
Contributor Author

I also think that renaming Create to New would be a bit more intuitive, since it's responsible for the dx new subcommand.

@jkelleyrtp
Copy link
Member

Thanks! Will take a look tomorrow - might be able to get it into the release.

Copy link
Member

@jkelleyrtp jkelleyrtp left a comment

Choose a reason for hiding this comment

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

Actually, all looks good, thank you!

@jkelleyrtp jkelleyrtp merged commit fca5a82 into DioxusLabs:main Nov 16, 2024
17 checks passed
@Andrew15-5
Copy link
Contributor Author

Oh, wow, OK. Great, but what about questions from #2822 (comment) and #2822 (comment)?

@jkelleyrtp
Copy link
Member

Do we need some sort of "workspace" dx new/dx init tests?

Eventually, sure, plus a whole slew of other tests for the CLI. But for now it's not a big deal - the code here isn't too crazy to make it worth not shipping it in 0.6.

I also think that renaming Create to New

We could but it doesn't seem terribly important to do that either.

@Andrew15-5
Copy link
Contributor Author

Andrew15-5 commented Nov 16, 2024

Alright, and the last question from the #2822 (comment). I guess slightly more up-to-date random crates isn't an issue either? I thought that this could add some potential problems, but maybe it's fine since all the current tests are passing.

We could but it doesn't seem terribly important to do that either.

Then I'll rename it later. Because even I was slightly confused when searching for the correct file. Had to look for create instead of new. Probably because it's been a while since I worked with the files.

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