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.

Sign up for GitHub

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

Jump to bottom

Add GetSpanContents engine call #12439

Merged
merged 1 commit into from
Apr 9, 2024
Merged

Add GetSpanContents engine call #12439

merged 1 commit into from
Apr 9, 2024

Conversation

devyn
Copy link
Contributor

@devyn devyn commented Apr 7, 2024 •
edited
Loading

Description

This allows plugins to view the source code of spans.

Requested by @ayax79 for implementing polars ls. Note that this won't really help you find the location of the span. I'm planning to add another engine call that will return information more similar to what shows up in the miette diagnostics, with filename / line number / some context, but I'll want to refactor some of the existing logic to make that happen, so it was easier to just do this first. I hope this is enough to at least have something somewhat useful show up for polars ls.

User-Facing Changes

  • Example plugin: added example view span command

Tests + Formatting

  • 🟢 toolkit fmt
  • 🟢 toolkit clippy
  • 🟢 toolkit test
  • 🟢 toolkit test stdlib

After Submitting

  • Add to plugin protocol reference

@devyn
Add GetSpanContents engine call
2e5ba47 
This allows plugins to view the source code of spans.

Requested by @ayax79 for implementing `polars ls`.
WindSoilder
WindSoilder reviewed Apr 7, 2024
View reviewed changes
crates/nu-plugin/src/plugin/context.rs
.engine_state
.get_span_contents(span)
.to_vec()
.into_spanned(self.call.head))
Copy link
Collaborator

Choose a reason for hiding this comment

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

into_spanned(self.call.head)) seems wrong to me, because we need to get content from given span.

So I think this should be into_spanned(span)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I wasn't really sure which to choose... But if the value were going to be returned from the command, it would be better for it to be spanned from the command than from the span of the source code.

It doesn't matter so much for Rust because this span is actually not used in the API. I just didn't want to keep creating unspanned variants of EngineCallResponse when there are perfectly usable Value types.

Anyway, the reasoning is that the span refers to the source code, but the source code string originates from the command that produced it

@sholderbach
Copy link
Member

While I see the need in the short term to get the existing command moved over to the plugin., I am a bit skeptical if "reflection-we-have-at-home" level introspection operations looking into the engine should be part of the plugin interface for stabilization.
For the purposes of the dataframe or more general PluginCustomValues we may want to narrow this to an API exposing purely the live scope of variables of concern (and later maybe other symbols). Anything that can be used will be used against us refining the internals.

But we can refine that in the upcoming releases.

@devyn
Copy link
Contributor Author

devyn commented Apr 8, 2024

I understand the reservation, but for the moment plugins have really no access to anything other than the spans that come from values.

Would you rather see something that takes an entire value and just spits out some metadata about where it came from, including the source code information?

This is already not perfect for dfr ls, which would probably prefer to show a little more metadata, so maybe we skip this for now, if you think that something based on metadata for values would be easier to commit to.

@sholderbach
Copy link
Member

I am fine with going ahead with this PR for now.

Maybe we can come up with a list of metadata that a plugin is reasonably interested in about its custom values and provide the necessary interface for that.

@fdncred
Copy link
Collaborator

fdncred commented Apr 9, 2024

Let's move forward for now. Thanks!

@fdncred fdncred merged commit 00b3a07 into nushell:main Apr 9, 2024
15 checks passed
@fdncred fdncred added pr:plugins This PR is related to plugins pr:api-change This PR should be mentioned in #api-updates channel on Discord labels Apr 9, 2024
@hustcer hustcer added this to the v0.93.0 milestone Apr 9, 2024
FilipAndersson245 pushed a commit to FilipAndersson245/nushell that referenced this pull request Apr 10, 2024
@devyn @FilipAndersson245
Add GetSpanContents engine call (nushell#12439)
a225661 
# Description
This allows plugins to view the source code of spans.

Requested by @ayax79 for implementing `polars ls`. Note that this won't
really help you find the location of the span. I'm planning to add
another engine call that will return information more similar to what
shows up in the miette diagnostics, with filename / line number / some
context, but I'll want to refactor some of the existing logic to make
that happen, so it was easier to just do this first. I hope this is
enough to at least have something somewhat useful show up for `polars
ls`.

# User-Facing Changes
- Example plugin: added `example view span` command

# Tests + Formatting
- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting
- [ ] Add to plugin protocol reference
NotTheDr01ds pushed a commit to NotTheDr01ds/nushell that referenced this pull request Apr 12, 2024
@devyn @NotTheDr01ds
Add GetSpanContents engine call (nushell#12439)
2e266a8 
# Description
This allows plugins to view the source code of spans.

Requested by @ayax79 for implementing `polars ls`. Note that this won't
really help you find the location of the span. I'm planning to add
another engine call that will return information more similar to what
shows up in the miette diagnostics, with filename / line number / some
context, but I'll want to refactor some of the existing logic to make
that happen, so it was easier to just do this first. I hope this is
enough to at least have something somewhat useful show up for `polars
ls`.

# User-Facing Changes
- Example plugin: added `example view span` command

# Tests + Formatting
- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting
- [ ] Add to plugin protocol reference
@devyn devyn deleted the get_span_contents-engine-call branch April 13, 2024 21:50
@devyn devyn mentioned this pull request Apr 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WindSoilder WindSoilder WindSoilder left review comments

Assignees
No one assigned
Labels
pr:api-change This PR should be mentioned in #api-updates channel on Discord pr:plugins This PR is related to plugins
Projects
None yet
Milestone
v0.93.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@devyn @sholderbach @fdncred @WindSoilder @hustcer