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: JSON adapter RFC #341

Closed
wants to merge 2 commits into from
Closed

Conversation

JamesWTruher
Copy link
Member

A proposal for automatically calling a function/script/command to convert text output to PowerShell objects.

drwxr-xr-x 10 james staff 320 Dec 6 12:05 .
```

Can or should something like this be done for PowerShell?
Copy link
Collaborator

Choose a reason for hiding this comment

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

An environmental variable is probably the easiest? If the main way to "opt-in" to this experience is to get these adapters then I think having a variable to opt out without getting rid of all the adapters is good too.

Another, rather redundant, option would be provide a PS cmdlet so folks could pipe adapted commands to, to unadapt them? i.e docker image ls | Out-Text or something like that.

Copy link
Member Author

Choose a reason for hiding this comment

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

add how to handle singular disablement into alternate considerations

### History

It's not clear whether history should be altered to show the added pipeline elements. We don't currently do that for `out-default` which can also add to the pipeline.
It should be relatively easy to determine whether the pipeline as the `app-json`, as `get-command app` will include the name of the json adapter.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm of the opinion we just save what is typed by the user and not added pipeline elements to avoid confusion. This could be another case for adding additional fields to history objects so show what is added to the pipeline.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Steven....From my perspective, I want to see what I typed in my history -- all the additional would confuse me as I did not type that. Should we have a way to show the underlying?


Can or should something like this be done for PowerShell?

## Alternatives
Copy link
Contributor

Choose a reason for hiding this comment

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

There is 2.5 year old proposal which cover all scenarios from the RFC and even more
PowerShell/PowerShell#13428
And as it was said there, it's great in combination with https://github.com/dotnet/command-line-api

@JamesWTruher JamesWTruher changed the title WIP: JSON adapter RFC JSON adapter RFC Mar 1, 2023
PS> MyCustomApp.exe -p1 MyString -p2 42 -p3 resource=blue
```

_and_ if `MyCustomApp-json` is available as a conversion too for `MyCustomApp.exe`,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_and_ if `MyCustomApp-json` is available as a conversion too for `MyCustomApp.exe`,
_and_ if `MyCustomApp-json` is available as a conversion tool for `MyCustomApp.exe`,

we could automatically construct the following:

```powershell
PS> MyCustomApp.exe -p1 MyString -p2 42 -p3 resource=blue | MyCustomApp-json -ParameterBlock @{ p1 = "MyString"; p2 = 42; p3 = "resource=blue" } | ConvertFrom-Json
Copy link
Member

Choose a reason for hiding this comment

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

Is there a case where the JSON adapter needs ConvrtFrom-Json -AsHashTable as in the case of an empty key or keys that differ only in casing

converter to automatically construct objects from tabular data could be called.
This would enable the user to set it once and later sessions would be able to use it.

#### PSDefaultNativeParameterValues
Copy link
Member

Choose a reason for hiding this comment

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

It's not clear to me if this section is actually proposing work to be done or just calling out a potential feature and problems with it. It seems like it's the latter which I would then recommend moving to Alternate Considerations which implies it's not in scope


A number of follow-on activities should be considered to bring continued improvement of the native command experience.

#### Improved TabCompletion
Copy link
Member

Choose a reason for hiding this comment

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

This seems orthogonal to json adapter?


There are two aspects of making this work well:

- Automatically adding `ConvertFrom-Json` to the pipeline following the native command to automatically convert `Json` to object
Copy link
Collaborator

Choose a reason for hiding this comment

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

How are we going to to know we need to automatically add the ConvertFrom-Json to the pipeline or not? Cause we dont need an adapter for example on line 37 to work

Copy link
Collaborator

Choose a reason for hiding this comment

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

Think may have answered on line 80 but does that mean still requiring a basic adapter to only call ConvertFrom-Json to know if it is used or not

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe clarify here its always the adapter and not ConvertFrom-JSON being put at the end

We already will tab-complete for file system items,
a small effort should be able to increase our coverage for other native executables.

#### Improved Formatting Tooling
Copy link
Member

Choose a reason for hiding this comment

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

For formatting, do TypeNames get automatically added to the resulting object? Maybe something like: <CommandName>.JsonAdapted so formatting can be built for it?


### Adaptation of Native Apps by Fully Qualified Path

Should the automatic json adaptation be suppressed when a fully qualified path (`/bin/ls` vs `ls`) is provided?
Copy link
Member

Choose a reason for hiding this comment

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

For now, whether full path or not should not affect insertion of json adapter if one is found

drwxr-xr-x 10 james staff 320 Dec 6 12:05 .
```

Can or should something like this be done for PowerShell?
Copy link
Member

Choose a reason for hiding this comment

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

PowerShell/PowerShell#3316 proposes new syntax for specifying env var, however, I would prefer just enhancing Start-Process. But what would this env var be if we chose this approach?

With stand-alone native applications, it means that providing for reasonable formatting will require an additional file.
A more dynamic approach for formatting would improve the experience for the user.

## Open Questions
Copy link

@dcaro dcaro Mar 8, 2023

Choose a reason for hiding this comment

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

How do we handle errors? For example, in the case the json is not formatted correctly?

This means that rather than just capturing the parameter name and default value, we will need to be able to run arbitrary code (ala scriptblock)
which includes the command line AST so the proper parameter values may be created.

However, with an approach which provides for this complexity, we can continue to simplify the experience for our users to from:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
However, with an approach which provides for this complexity, we can continue to simplify the experience for our users to from:
However, with an approach which provides for this complexity, we can continue to simplify the experience for our users from:

### Parser Modification

The parser does not make any disambiguation of the type of command to be executed.
It simple identifies the token which _will_ be executed at run time.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
It simple identifies the token which _will_ be executed at run time.
It simply identifies the token which _will_ be executed at run time.

. . .
```

### Follow-on Work
Copy link
Member

Choose a reason for hiding this comment

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

Define tag for publishing modules to PSGallery that includes json adapters

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would there be anything stopping someone from making an adapter that is just a keylogger or something that steals the users data?


- Automatically adding `ConvertFrom-Json` to the pipeline following the native command to automatically convert `Json` to object
- Automatically adding a parameter to a native command similarly to what `$PSDefaultParameterValue` does for cmdlets

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we maybe do a call out somewhere in the doc to talk about the user experience being just installing a PowerShell module that contains these adapters?

@JamesWTruher JamesWTruher changed the title JSON adapter RFC WIP: JSON adapter RFC Mar 21, 2023
Addressed a number of comments provided during first review.
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

7 participants