Remove hard-coded support for mainargs.Leftover/Flag/SubParser to support alternate implementations #62
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
This was referenced Apr 27, 2023
lihaoyi
changed the title
lihaoyi
changed the title
|
Rebased this on top of https://github.com/com-lihaoyi/mill I renamed |
lefou
reviewed
Apr 28, 2023
lefou
reviewed
Apr 28, 2023
build.sc
Outdated
| @@ -19,7 +19,7 @@ val scalaJSVersions = scalaVersions.map((_, "1.10.1")) | |||
| val scalaNativeVersions = scalaVersions.map((_, "0.4.7")) | |||
|
|
|||
| trait MainArgsPublishModule extends PublishModule with CrossScalaModule with Mima { | |||
| def publishVersion = VcsVersion.vcsState().format() | |||
| def publishVersion = "0.5.0-M1" | |||
There was a problem hiding this comment.
Why is that? Once you tag the commit, it should happen automatically.
There was a problem hiding this comment.
Oh that's just leftover from manual testing using publishLocal, will revert before landing
lihaoyi
added a commit
that referenced
this pull request
Apr 29, 2023
…#66) Should fix #58 and #60 Previously, we allowed any arg to take positional arguments if `allowPositional = true` (which is the case for Ammonite and Mill user-defined entrypoints.), even `mainargs.Flag`s. for which being positional doesn't make sense. ```scala val positionalArgSigs = argSigs .filter { case x: ArgSig.Simple[_, _] if x.reader.noTokens => false case x: ArgSig.Simple[_, _] if x.positional => true case x => allowPositional } ``` The relevant code path was rewritten in #62, but the buggy behavior was preserved before and after that change. This wasn't caught in other uses of `mainargs.Flag`, e.g. for Ammonite/Mill's own flags, because those did not set `allowPositional=true` This PR tweaks `TokenGrouping.groupArgs` to be more discerning about how it selects positional, keyword, and missing arguments: 1. Now, only `TokenReader.Simple[_]`s with `.positional` or `allowPositional` can be positional; `Flag`s, `Leftover`, etc. cannot 2. Keyword arguments are limited only to `Flag`s and `Simple` with `!a.positional` Added `mainargs.IssueTests.issue60` as a regression test, that fails on main and passes on this PR. Existing tests all pass
lihaoyi
added a commit
to com-lihaoyi/mill
that referenced
this pull request
May 1, 2023
Fixes #1948. Should probably be reviewed concurrently with com-lihaoyi/mainargs#62 which it depends on # Major Changes 1. Pull in com-lihaoyi/mainargs#62, which allows MainArg's "aggregate leftover arguments" logic to apply to custom types and not be hardcoded to `mainargs.Leftover` 2. Update MainArgs integration code in this repo to accomadate the changes 3. Replace the `def run(args: String*)` with `def run(args: Task[Args] = T.task(Args()))`. # Minor Changes 1. I moved the `import mill.main.TokenReaders._` into the `Discover` macro, so we can stop manually importing it everywhere. 2. I moved all the aliases from `import mill.define._` to `import mill._`. `Task[_]` in particular is a type I believe we should start encouraging people to use more often - e.g. annotating all command parameter types as `Task[_]`s - and so it should be in the default `import mill._` without needing a separate import. 3. Added a direct dependency to `com.lihaoyi::pprint`, since the implicit dependency in `mainargs` was removed in 0.5.0 due to being unnecessary # Testing 1. All existing tests pass, at least those I've run locally so far (`./mill -i -j 3 example.__.local.test`, can't use CI until the mainargs PR is published). This includes unit tests that call `run` programmatically and integration/example tests that call `run` via the CLI. 4. Added a new example test `5-module-run-task`, that demonstrates and explains how to use a `ScalaModule`'s `run` method to implement a `task`, passing in input parameters from upstream tasks and using the output (in this case generated files) in downstream tasks # Notes 1. There is still some boilerplate around `run`, both defining it `args: Task[Args] = T.task(Args())` and calling it programmatically, `run(T.task(Args(...)))`. For now I haven't figured out how to reduce this, as we already "used up" our implicit conversion budget with the `T.*{...}` macros. Probably can be done, although it won't be quite as low-boilerplate as the original `args: String*` syntax. Maybe in a follow-up PR 2. The example test `5-module-run-task` is something I've wanted to be able to write for years now. Nice to finally be able to do it without having to piece together a `Jvm.runSubprocess()` call myself! 3. `5-module-run-task` is still a bit clunkier than I was hoping. Having to `def barWorkingDir` beforehand to pass it to both the `bar.run` and the `def sources` override is weird and unintuitive. Maybe we can change the `def run` return type to make it easier to return the `T.dest` folder or other metadata out of the body of `def run` so it can be used by downstream tasks? e.g. making `def run` return `Command[PathRef]` rather than `Command[Unit]` 5. IMO we should recommend that all CLI arguments to `T.command`s come in the form of `Task[T]`s, unless there's concrete reasons otherwise (e.g. we want to dynamically change `T.command` implementations based on the input). That would maximise the ability to re-use commands in the body of of tasks, which is something people probably want --------- Co-authored-by: Tobias Roeser <[email protected]>
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.
This PR moves the handling of
mainargs.Leftover/Flag/SubParserfrom a hard-codedArgSigthat only works withmainargs.Leftoverormainargs.Flag, to properties of theTokensReaderthat can be configured to work with any custom type.Should probably be reviewed concurrently with com-lihaoyi/mill#1948, which is the motivation for this PR: we want to be able to define a CLI entrypoing taking
mill.define.Task[mainargs.Leftover[T]]or equivalent, which is currently impossible due to the hard-coded nature ofmainargs.Leftover(andmainargs.Flagetc.)Major Changes
ArgReaderis eliminated andArgSigis greatly simplified to a single type with no subtypes or type parametersTokensReaderis split into 5primary sub-types -.Simple,Constant,.Flag,.Leftover, and.Class. These roughly mirror the original{ArgSig,ArgReader}.{Simple,Flag,Leftover,Class}case classes. The 5 sub-classes control behavior throughRenderer.scala/Invoker.scala/TokensGrouping.scalain the same way.The major effect of moving the logic from
{ArgSig,ArgReader}toTokensReaderis that they now are no longer hard-coded to work withmainargs.{Flag,Leftover,Subparser}types. Now, anyone who has a custom typeFoocan choose whether they want to define aTokensReader.Simplefor it, or whether they want to define aTokensReader.LeftoverorTokensReader.Flag. Similarly, people can define their ownTokensReader.Classrather than relying on the default implementation inmainargs.ParserForClass.Testing
Tested with two new flavors of
VarargsTests(now renamedVarargsBasedTests:VarargsWrappedTeststhat exercises using a custom wrapper type to define a main entrypoints that takesWrapper[mainargs.Leftover[T]],VarargsCustomTeststhat replacesmainargs.Leftover[T]entirely and defines main entrypoints that takeWrapper[T]Added a
ConstantTests.scalato exercise the code path, which was previously thenoTokenscodepath and un-tested in this repoAll existing tests pass
Notes
I chose to remove the type params from
ArgSigbecause they weren't really paying for their complexity; most of the time we were passing aroundArgSig[_, _]s anyway, so we weren't getting type safety, but they nevertheless caused tons of headaches trying to get types to line up. The un-typeddefault: Option[Any => Any], reader: TokensReader[_]isn't great, but it's a lot easier to work with and TBH not really much less type-safe than the status quoBecause
ArgSigandTokensReadernow have a circular dependency on each other (viaTokensReader.Class->MainData->ArgSig->TokensReader), I moved them into the same file. This both makes the acyclic linter happy, and also kind of makes sense since they're now part of the same recursive data structure (v.s. previouslyArgSigwas the recursive data structure withTokensReaders hanging off of each node)The new structure with
TokensReaderas asealed traitwith 5 distinct sub-types is different from what it was before, withTokensReaderas a singleclasswith a grab-bag of all possible fields and callbacks. I thought thesealed traitapproach is much cleaner here, since they reflect exactly the data necessary 4 different scenarios we care about, whereas otherwise we'd find some fields meaningless in some cases e.g.Flaghas no meaningful fields,Leftoverdoesn't care aboutnoTokensoralwaysRepeatableorallowEmpty, etc.