-
Notifications
You must be signed in to change notification settings - Fork 21
Send OpenFile
request with kind
field
#890
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
Conversation
/// How to interpret the 'file' argument: as a file path or as a URI. If | ||
/// omitted, defaults to 'path'. | ||
pub kind: OpenEditorKind, |
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 feel like the argument names are a little mixed up, I feel like if we are generalizing, it should become
// The path of the file or URI to open
pub path: String
// How to interpret the `path` argument: as a file or a URI.
// If omitted, defaults to `File`
pub kind: OpenEditorPathKind
#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, strum_macros::Display, strum_macros::EnumString)]
pub enum OpenEditorPathKind {
#[serde(rename = "file")]
#[strum(to_string = "file")]
File,
#[serde(rename = "uri")]
#[strum(to_string = "uri")]
Uri
}
i.e. to me the ambiguous concept is a path, which is specialized to either a file or a uri.
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.
The positron side of this PR seems to agree with me, where you have
if (ed.kind === 'uri') {
file = URI.parse(ed.file);
} else {
file = URI.file(ed.file);
}
i.e. you have an arbitrary path
, if URI you parse()
it, otherwise if File you file()
it
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.
That makes sense
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.
Although I think I'd prefer not to change path
as this would likely require updating the Python side. The current setup is backward compatible.
e462705
to
5bdbe5d
Compare
5bdbe5d
to
d8a8f95
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.
Approving again as a way to note that I looked this over once more
93df8c6
to
3e05369
Compare
Companion PR for posit-dev/positron#8816
QA Notes
See linked PR.