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

Inspect supports alias #4221

Merged
merged 1 commit into from Jun 25, 2018

Conversation

Projects
None yet
2 participants
@gpoirier

gpoirier commented Jun 19, 2018

Fixes #2881

@eed3si9n eed3si9n added the ready label Jun 19, 2018

@gpoirier

This comment has been minimized.

gpoirier commented Jun 19, 2018

I signed the CLA

@eed3si9n eed3si9n added the hackathon label Jun 20, 2018

@eed3si9n

This comment has been minimized.

Member

eed3si9n commented Jun 21, 2018

@gpoirier Thanks for the contribution!

}
val spacedModeParser: (State => Parser[Mode]) = (s: State) => {
val spacedModeParser: State => Parser[Mode] = (_: State) => {
val default = "-" ^^^ Details(false)

This comment has been minimized.

@gpoirier

gpoirier Jun 21, 2018

@eed3si9n @dwijnand Is it a decent solution to wanting inspect the key named 'actual' with Details(false)? I can leave it out or renamed - to a different name if preferred.

This comment has been minimized.

@eed3si9n

eed3si9n Jun 21, 2018

Member

It's a bit ironic since - is an actual command name in sbt 0.13 to do early processing, like -warn, but I think inspect - somekey is a decent escaping solution.

@gpoirier

This comment has been minimized.

gpoirier commented Jun 21, 2018

Good point, I missed the help inspect message. I'll update it, and I should also add something about the - mode if you guys want that in? https://github.com/gpoirier/sbt/blob/issues/2881/main/src/main/scala/sbt/internal/Inspect.scala#L36

I'll also have to figure out how to run the scripted tests locally so I don't rely on travis to know if my fix worked - because my first attempt didn't.

@gpoirier

This comment has been minimized.

gpoirier commented Jun 21, 2018

It seems like this PR should be linked to the issue #2881, but I cannot figure out how to do that.

@gpoirier

This comment has been minimized.

gpoirier commented Jun 22, 2018

I updated the help text, does it seems appropriate?

@eed3si9n

This comment has been minimized.

Member

eed3si9n commented Jun 22, 2018

@gpoirier

The beginning of the help text says:

|	For a plain setting, the value bound to the key argument is displayed using its toString method.
|	Otherwise, the type of task ("Task" or "Input task") is displayed.

Maybe it should be updated a bit to reflect your main change. For example:

For a plain setting, the value bound to the key argument is displayed using its toString method.
For an alias, the command bound to the alias is displayed.
Otherwise, the type of the key ("Task" or "Input task") is displayed.
@gpoirier

This comment has been minimized.

gpoirier commented Jun 22, 2018

With the help change above, is it instead of the text I added at the bottom? The main reason I added it at the bottom was that the change apply to all of the modes.

@eed3si9n

This comment has been minimized.

Member

eed3si9n commented Jun 22, 2018

Either works for me in terms of the text.

@eed3si9n

This comment has been minimized.

Member

eed3si9n commented Jun 22, 2018

Overall LGTM.
Could you squash the commits please?

Guillaume Poirier
Adding minimal support for commands in inspect
There's also a special case for aliases that will try to resolve
the target of the alias to a task key if possible and display the
output of that key if found.

see #2881
@gpoirier

This comment has been minimized.

gpoirier commented Jun 22, 2018

It's squashed to a singe commit rebased on the current HEAD of 1.x.

@eed3si9n

This comment has been minimized.

Member

eed3si9n commented Jun 23, 2018

Awesome.

@eed3si9n eed3si9n added this to the 1.2.0 milestone Jun 23, 2018

@eed3si9n eed3si9n changed the title from Issues/2881 to Inspects supports alias Jun 23, 2018

@eed3si9n eed3si9n changed the title from Inspects supports alias to Inspect supports alias Jun 23, 2018

@eed3si9n

This comment has been minimized.

Member

eed3si9n commented Jun 23, 2018

I am testing it locally, and seeing something like this:

sbt:helloworld> inspect show
[error] Not a valid project ID: show
[error] Expected ':'
[error] Not a valid key: show (similar: showTiming, sLog, ps)
[error] inspect show
[error]             ^
sbt:helloworld> inspect +
[error] Expected whitespace character
[error] Expected '-'
[error] Expected 'tree'
...

Is this the expected result for commands?

@eed3si9n

See comment

@gpoirier

This comment has been minimized.

gpoirier commented Jun 24, 2018

Those two (+ and show) aren't in the list from State#definedCommands, which is what @dwijnand asked me to use. As far as I can tell, show doesn't seem to be technically be a command, or at least I can't find it by browsing the source if it's the case. Also, I can only use the SimpleCommands, the ArbitraryCommands doesn't expose a name.

The two error messages you listed are also different for reasons that already existed. One of the parser for the keys is looking at IDs before trying to match the result to a key. Which I'm realizing right now might be an issue, since technically keys could have names that are not valid IDs. But this PR isn't changing that behavior.

@gpoirier

This comment has been minimized.

gpoirier commented Jun 25, 2018

@eed3si9n do you still want me to change something? If so, what should it be?

@eed3si9n

ok. Let's merge this as is. Since adding support for alias was the original goal, so this PR does the job.

@eed3si9n eed3si9n merged commit 8d15279 into sbt:1.x Jun 25, 2018

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@eed3si9n eed3si9n removed the ready label Jun 25, 2018

@gpoirier

This comment has been minimized.

gpoirier commented Jun 26, 2018

Should a new ticket be open for the part that's not related to adding alias support then? What was your issue though, the fact that show and + weren't seen as commands or the error message? If it's the former, I don't think it can easily be solved.

@eed3si9n

This comment has been minimized.

Member

eed3si9n commented Jun 27, 2018

I guess I was hoping that inspect somehow recognizes + as a command. I think it's ok as is.

We made a survey form to collect feedback on the contribution process. Please fill it out when get you get a minute. https://goo.gl/forms/LDPlTl9Wso7YwEri2

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