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 support for Elixir #10

Merged
merged 36 commits into from
Jan 16, 2018
Merged

Add support for Elixir #10

merged 36 commits into from
Jan 16, 2018

Conversation

stevedomin
Copy link
Contributor

From @greysteil's PR gocardless/bump-core#57

A starting point for adding Elixir support.

Next step would be to write a parser for the mix.lock file, in Elixir. The JavaScript helper run task and parser are the best places to start for understanding how shelling out to helpers works. We'll probably want to piggy back off of Hex as much as possible here, and include it as a dependency (like we do with Yarn).

Once an Elixir parser is written, the pending specs in spec/bump/file_parsers/elixir/hex_spec.rb should pass. At that point, let's talk about adding the UpdateChecker, FileUpdater and MetadataFinder classes.

@greysteil greysteil force-pushed the master branch 2 times, most recently from 064a304 to ca403cc Compare July 7, 2017 10:00
@@ -0,0 +1,65 @@
defmodule MixfileParser do
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know enough Elixir to know if this is possible, but is there any way we could piggy-back off of the parsers within Mix itself, rather than having to roll our own? For example, in Python we can just nick the parse_requirements method from pip.req.req_file (see https://github.com/hmarr/dependabot-core/pull/17/files#diff-894615e139b303f40cb5c0d8ed295021R7)

@greysteil
Copy link
Contributor

Looks like a good start! Anything I can do to help?

@greysteil
Copy link
Contributor

Update: @stevedomin and I are going to make this happen on Wednesday 18th. 🎉

@greysteil
Copy link
Contributor

@stevedomin - what's the best next step on this? Don't want it to lose momentum. Can you push whatever you've got on your machine and add a TODO list of what's remaining?

@stevedomin
Copy link
Contributor Author

@greysteil please before pulling this branch again can you make sure you checkout your local branch to a temp branch, I accidentally removed your commits around metadata.

@stevedomin
Copy link
Contributor Author

Thanks @greysteil! :)

@stevedomin
Copy link
Contributor Author

@greysteil got the UpdateChecker close to finished. Once this is done there is only the FileUpdater left, right?

@greysteil
Copy link
Contributor

That's awesome! And yep, that sounds right to me. Want me to take a look through what's here now?

end
end

# %Mix.Dep{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is going to go away before we merge into master

@stevedomin
Copy link
Contributor Author

Yep, go for it!

@greysteil
Copy link
Contributor

greysteil commented Jan 9, 2018

I'm going to need to do a big rebase on this (makes sense for it to be me as I know the changes in the master best). Can you let me know when everything you've got is pushed up, and I'll get it done?

@greysteil greysteil force-pushed the add-elixir branch 4 times, most recently from b30f9ff to 843ade3 Compare January 10, 2018 11:48
@greysteil
Copy link
Contributor

greysteil commented Jan 10, 2018

^^^ It worked!!

OK, remaining TODOs:

  • A requirements updater (for the update checker)
  • More specs for the parser (e.g., what does it do when no versions is specified? What does it do for git dependencies)
  • A FileUpdater

@stevedomin - are you OK to do the FileUpdater? I'll do the requirement updater (it can be in Ruby).

Also, what's happening under the hood in the UpdateChecker? Is Dependabot doing a full install, or just piggy-backing off of mix's resolution logic. Because if it's the former it could be really slow...

@stevedomin
Copy link
Contributor Author

Awesome @greysteil, thank you so much!

Right now, it's doing a full install but I'm working on a version without it. I wanted to get something out of the door, been trying to get it working without the install for way too long.

We can definitely make it work without doing a full install but I'll need to make some changes to the Mix codebase itself though.

Are you ok with getting a working version like this and then once it's out we can figure out to do this without the full install?

I'll do the FileUpdater and add specs to the parser.

@greysteil
Copy link
Contributor

greysteil commented Jan 11, 2018

Makes sense to me - if you get the FileUpdater done I’m 👍 on shipping

@stevedomin
Copy link
Contributor Author

Awesome!

@greysteil
Copy link
Contributor

greysteil commented Jan 11, 2018

RequirementsUpdater is done, and pretty solid.

One question - for equality, is "== 1.0.0" a valid specifier? And if so, is "= 1.0.0" valid or invalid? We're going to need to handle that if so... It is, and it's fixed. 🎉

@greysteil
Copy link
Contributor

@stevedomin - I've added a FileUpdater, and done the mixfile updating logic. Just needs your Elixir code to do the lockfile updating and this will be basically good to do.

@greysteil
Copy link
Contributor

@stevedomin - added a couple of failing specs for you. Only thing needed now is the Elixir code to update the lockfile, which I think you can basically pinch from what you wrote for the UpdateChecker...

@greysteil greysteil changed the title [WIP] Add support for Elixir Add support for Elixir Jan 16, 2018
@greysteil greysteil merged commit f2c6852 into master Jan 16, 2018
@greysteil greysteil deleted the add-elixir branch January 16, 2018 20:15
@greysteil
Copy link
Contributor

@stevedomin - I think this is all working, so I'm going to merge and release. Would love your help improving and testing it, though!

@stevedomin
Copy link
Contributor Author

Awesome work and thanks @greysteil. I'll add it to my repos tomorrow.

And I'll check the codebase to see what we can improve. At the very least I'd like to change UpdateChecker so that it doesn't pull the dependencies.

@greysteil
Copy link
Contributor

That would be a huge improvement! 🎉

@greysteil
Copy link
Contributor

@stevedomin could I get an IP assignment comment from you, just so it's explicit what your intentions were with this PR? Ideally the following text would be amazing:

I'm gifting the IP of the contents of this PR and all previous and future contributions to dependabot-core to Dependabot for it to be used however Dependabot wishes.

Thanks. ❤️

@stevedomin
Copy link
Contributor Author

I'm gifting the IP of the contents of this PR and all previous and future contributions to dependabot-core to Dependabot for it to be used however Dependabot wishes.

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