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

Add mix deps.add task #10291

Closed

Conversation

TheFirstAvenger
Copy link
Contributor

Add the mix deps.add task. Supports options such as:

mix deps.add foo --version 1.2.3
mix deps.add foo (pulls latest version from hex)
mix deps.add foo --no-runtime
mix deps.add foo --only test --only dev
mix deps.add foo --path ../foo

Note: determining latest version from Hex is not yet implemented, defaults to >= 0.0.0. Would like to implement it before merging as it is one of the primary benefits of this feature. Looking to implement this using existing logic if possible instead of falling back to :httpc call.

@hauleth
Copy link
Contributor

hauleth commented Aug 31, 2020

The only problem I see there is that such util will be very Hex-centric. Maybe it should be moved to the Hex itself? So it would be mix hex.add instead.

@josevalim
Copy link
Member

Hi @TheFirstAvenger, can you please send to for discussion to the mailing list? I believe it also has been discussed before.

I personally worried about going the string matching route. There are just too many things that can go wrong. For example, someone can even define all deps inline such as [{:foo, :bar}, {:baz, :bat}]. And this makes me skeptical about adding such solution to core. Ideally we would do something that hooks into the code formatter engine but even that has other complexities.

@TheFirstAvenger
Copy link
Contributor Author

The only problem I see there is that such util will be very Hex-centric. Maybe it should be moved to the Hex itself? So it would be mix hex.add instead.

I would lean towards mix deps.add because although pulling the version from Hex is a big feature, when specifying a version or path, there is no hex interactions at all, so mix hex.add foo path: ../foo is a bit confusing. I would additionally probably want to add a github option next, which also would be confusing if it were mix hex.add foo --github foo-project/foo instead of mix deps.add ....

@josevalim
Copy link
Member

Closing per the above, thank you!

@josevalim josevalim closed this Aug 31, 2020
@hauleth
Copy link
Contributor

hauleth commented Aug 31, 2020

Just following my comment there:

@TheFirstAvenger what about private repos? Or custom repos? What about other SCMs like Git? Or even custom ones? In current form mix deps.add seems very Hex centric.

@wojtekmach
Copy link
Member

If we were to support it, I think the usage could be similar to archive.install/escript.install, that is:

mix deps.add hex foo "~> 1.0"
mix deps.add hex foo "~> 1.0" --organization acme
mix deps.add github foo org/repo
# etc

On the implementation side, we'd probably add a new callback to https://hexdocs.pm/mix/Mix.SCM.html.

Usage aside, by far the biggest problem to solve is manipulating the mix.exs file, instead of working on strings we want to somehow work on code. As José alluded to, the most viable option seems to somehow plug into the formatter engine, we read some code, manipulate it, format it, and write it back.

I have a proof of concept for such solution here: https://github.com/wojtekmach/fix (and deps.add "backend" here: wojtekmach/fix@895c76b). As again José mentioned the complexity with the formatter engine is it has a custom AST (a "formatter AST" if you will) that is private and can change any time so it's not a stable foundation to built upon. What I'm exploring next is a higher level tool, along the lines of https://engineering.fb.com/open-source/retrie/.

Worth mentioning that even if we have a tool that can automatically rewrite AST, we won't be able to support all use cases for adding deps. Someone could have:

defp project() do
  [
    deps: deps(Mix.env())
  ]
end

defp deps(:dev) do
  [...]
end

and the tool couldn't handle that, but that's probably OK to error in that case. Vast majority of cases would use "regular" deps.

@stefanchrobot
Copy link
Contributor

What problem is this feature trying to address? If this is about querying Hex then something like mix hex.query would be much easier to implement. Alternatively, the mix deps.add task could just print out the lines that need to be added to mix.exs (similar to how some generators work).

@hauleth
Copy link
Contributor

hauleth commented Sep 1, 2020

@stefanchrobot there is mix hex.info that prints such information already and you can use mix hex.search for searching for packages.

@TheFirstAvenger
Copy link
Contributor Author

To answer "What problem is this feature trying to address?", I am picturing Elixir tutorials being more approachable by being able to say something like:

mix new my_app
cd my_app
mix deps.add ecto
mix deps.add ecto_sql
mix deps.add faker --only :test

and get new users into writing code faster with less friction or possible confusion by having to parse through a mix.exs file before being able to do anything. For more advanced users, personally, I add things like mix_test_watch and dialyxir all the time when jumping into different OSS libraries to start contributing. Being able to do this from the command line after cloning without having to crack open mix.exs, figure out the latest version, and type it out manually would be a much nicer experience.

Keying off what @stefanchrobot mentioned about simply outputting the lines to be added, I updated the branch to do this in cases where mix.exs is not formatted or the deps function could not be identified. I think this would mitigate the concern about the current string parsing method, and would allow us to add this feature sooner, and then in the future improve the detection based on work like what @wojtekmach mentioned he is pursuing if/when it is available.

@hauleth On the idea that this is centered around hex, the only integration with hex is determining the latest version available, and since hex is already the default thing that elixir does when it sees a version: string, it makes sense that a deps.add would default to determining the latest version using that same source. Adding the ability to pull that version from wherever the end user wants would be a great feature enhancement. I would want to allow adding any valid deps option via this command. In that spirit, I also added github/ref/tag as an option in this pass.

If you would like to continue discussions on this, I created a PR in my own fork with the new changes: TheFirstAvenger#2

@stefanchrobot
Copy link
Contributor

@TheFirstAvenger

To answer "What problem is this feature trying to address?", I am picturing Elixir tutorials being more approachable by being able to say something like (...) and get new users into writing code faster with less friction or possible confusion.

I think this could be better served by integrating this into mix new:

mix new my_app --dep ecto --dep ecto_sql

It gets hairy when you want to pass more options. The upside is that the code generation will always work since it's starting with a clean slate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants