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

WIP Convert CLI argument parsing to use latest version of ammonite+mainargs #1008

Merged
merged 14 commits into from
Nov 24, 2020

Conversation

lihaoyi
Copy link
Member

@lihaoyi lihaoyi commented Nov 23, 2020

This replaces the copy-paste logic handling both Mill's own CLI argument parsing as well as the CLI argument parsing of user-defined T.commands to instead rely on the https://github.com/lihaoyi/mainargs implementation. This is similar to the earlier PR that performed such a migration in the lihaoyi/Ammonite repo com-lihaoyi/Ammonite#1126

Where the semantics of Mill's own CLI parsing and T.command CLI parsing differ, we pass in the corresponding flags to handle the difference, e.g. allowPositional = false for Mill's own parameters and allowPositional = false for T.commands.

Since the MainArgs library is standalone, we also cut the dependency on Scopt since it is now unnecessary. Our existing scopt.Reads are all replaced by mainargs.TokensReader

Things compile, haven't tested

@lihaoyi lihaoyi changed the title First pass at converting everything to use latest version of ammonite+mainargs WIP Convert CLI argument parsing to use latest version of ammonite+mainargs Nov 23, 2020
@lefou
Copy link
Member

lefou commented Nov 24, 2020

Looks good to me. I haven't used the new mainargs yet, so I can't say if it will fit all later needs, but given that it is already used in ammonite it is ok. (I did some local experiments with a single entry point main class, to avoid the logic in the start script, which also causes trouble for mill installations via coursier. But that solution needs to be done in Java and also requires us to parse the cmdline params in Java. There are very lightweight parsers in Java available as well, but it would means yet another change or some duplication.)

About the Github Actions setup: I already have PR #993 open using a test matrix and also working for scala native tests, which waits for your input regarding the publishing step.

@lihaoyi
Copy link
Member Author

lihaoyi commented Nov 24, 2020

Yeah I expect to merge this into master and try it out for a bit before cutting a stable release.

The mainargs code is pretty raw - took some surgery to extract it from ammonite, and then some re-organization to pretty it up and make it re-usable - but hopefully it's 99% there and we can manually suss out the last 1% before cutting a stable release of Mill + Ammonite

Sorry I didn't notice #993; I kind of hacked together the github actions config here from first principles. I'll probably merge this as is once i get things working, but feel free to replace it after if you think it could be done better somehow!

@lefou
Copy link
Member

lefou commented Nov 24, 2020

@lihaoyi , looks like you have disabled Travis-CI for the whole mill repository. Can you split up this PR in case you want to stabilize the mainargs migration longer, so that we can merge the Github Actions part before? We currently have other PRs awaiting merges which no longer get CI builds.

@Ammonite-Bot
Copy link
Collaborator

Ammonite-Bot commented Nov 24, 2020 via email

@lihaoyi
Copy link
Member Author

lihaoyi commented Nov 24, 2020

@lefou I've turned travis back on

@lihaoyi
Copy link
Member Author

lihaoyi commented Nov 24, 2020

All tests pass, merging this. Hopefully the post-merge logic passes too...

@lihaoyi lihaoyi merged commit d6dd9f6 into master Nov 24, 2020
@lefou lefou added this to the after 0.8.0 milestone Nov 24, 2020
@lefou lefou deleted the mainargs branch November 25, 2020 15:45
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.

None yet

3 participants