-
-
Notifications
You must be signed in to change notification settings - Fork 366
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
Show workers in resolve / show / inspect too #3580
Show workers in resolve / show / inspect too #3580
Conversation
818d8c9
to
0c7792e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
0c7792e
to
bdf56db
Compare
What's the motivation for this change? Workers aren't of any direct use from the cli. OTOH, to clean them up, they need to be discoverable. Is it that? |
Yes, that's for #3579 |
I still find it handy to be able to create them from the command-line. I remember having been surprised that wasn't possible when I was discovering Mill, a few years ago. |
I was wondering why you would want this, but that's mostly from my knowledge about existing worker implemntations. With the option to start and stop workers from the CLI, it's also possible for users to repurpose workers for other jobs in their build, like dev servers, which we would otherwise use |
5e478c2
to
bb851e3
Compare
I'm fine with this change in principle, as making For
I think for the purposes of this PR I'm happy to move forward, but we should flesh out the integration tests a bit both for correctness as well as clarity in what we expect the behavior to be, and where the limits are
|
There are already tests for |
I guess that's fine. That's the behavior I'd expect as a user. Running it with |
In the current state of this PR, we get an empty JSON object, but showing a string containing |
bb851e3
to
9b00579
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks great. Three things before merging:
- Let's flesh out the
show
output so it's a bit more informative than{}
; once we start showing workers inresolve
, people will start trying to poke at them viainspect
andshow
.inspect
already works as your tests demonstrate, butshow
is also a common thing to use so we should make it informative. How about something like this
{
"worker": "<worker-selector>",
"toString": "<worker.toString>",
"inputHash": 123456789
}
-
Should we leave a
<worker-name>.json
file on disk containing the snippet above? That would make theshow
output and the "dig through files on disk" output more consistent, so anyone digging through the build usingcat
orless
instead ofmill show
would be able to inspect workers as well -
Fill out the PR description summarizing the current state of the PR
9b00579
to
cfcd3a2
Compare
@lihaoyi I think your 3 points should be addressed |
This PR generalizes some
Target
-related stuff inResolve
to allNamedTask
s with no arguments (targets and workers), and makes sureresolve
/show
/inspect
work fine with workers. In particular, runningshow
on a worker now prints something like