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

Add `editorFileExtension` when clause context #34889

Merged
merged 4 commits into from Oct 10, 2017

Conversation

@Krzysztof-Cieslak
Copy link
Contributor

@Krzysztof-Cieslak Krzysztof-Cieslak commented Sep 23, 2017

This adds file extension when clause context.

Some languages are using same languageId for multiple different types of files (with different extensions). It may be useful to let developers define command by file extension, and not only by languageId. For example in F#, we have single languageId (fsharp) for all different types of files (.fs - normal F# files, .fsx - F# script files, .fsi - F# signature files). We would like to be able to display some commands, or menu entries only for one of the types.

@vbfox
Copy link
Contributor

@vbfox vbfox commented Sep 23, 2017

👍

@alexdima alexdima assigned jrieken and unassigned alexdima Sep 27, 2017
@jrieken jrieken added this to the October 2017 milestone Sep 27, 2017
@jrieken
Copy link
Member

@jrieken jrieken commented Sep 27, 2017

Great we want that, a dupe is already on the October plan

Copy link
Member

@jrieken jrieken left a comment

There is also https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/common/resources.ts#L23 which is used in the explorer and in editor that aren't using the code editor widget. An analog context key, like resourceExtname, is also needed

@@ -28,6 +28,7 @@ export namespace EditorContextKeys {

// -- mode context keys
export const languageId = new RawContextKey<string>('editorLangId', undefined);
export const fileExtension = new RawContextKey<string>('editorFileExtension', undefined);

This comment has been minimized.

@jrieken

jrieken Sep 27, 2017
Member

Don't use File as it might not a file, maybe editorResourceExtension, editorExtension, or editorExtname?

@@ -111,6 +114,7 @@ export class EditorModeContext extends Disposable {
return;
}
this._langId.set(model.getLanguageIdentifier().language);
this._fileExtension.set(model.uri.path.split('.').pop());

This comment has been minimized.

@jrieken

jrieken Sep 27, 2017
Member

😱 use paths.extname

}

set(value: URI) {
this._resourceKey.set(value);
this._schemeKey.set(value && value.scheme);
this._filenameKey.set(value && basename(value.fsPath));
this._langIdKey.set(value && this._modeService.getModeIdByFilenameOrFirstLine(value.fsPath));
this._extensionKey.set(value && paths.extname(value.fsPath));
}

reset(): void {

This comment has been minimized.

@Krzysztof-Cieslak

Krzysztof-Cieslak Sep 27, 2017
Author Contributor

@jrieken. an reason why _filenameKey is not reseted here?

This comment has been minimized.

@jrieken

jrieken Sep 28, 2017
Member

I'd say that is bug cc @bpasero

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@Krzysztof-Cieslak

Krzysztof-Cieslak Oct 6, 2017
Author Contributor

Should I add this missing reset in this PR?

@Krzysztof-Cieslak
Copy link
Contributor Author

@Krzysztof-Cieslak Krzysztof-Cieslak commented Sep 27, 2017

OK, I renamed context to editorExtension and added respective resourceExtension. Also used paths module in this one problematic place (didn't know before that you have those helpers)

@Krzysztof-Cieslak
Copy link
Contributor Author

@Krzysztof-Cieslak Krzysztof-Cieslak commented Sep 27, 2017

I just have one question, not connected to this particular PR but to contributing in general. Whenever I try to commit any changes they are failing with hygiene check... which would be OK, but the only reported errors are Missing or bad copyright statement on each file I change. Which seems wrong, and I'm forced to do --no-verify on each commit.

image

Copy link
Member

@jrieken jrieken left a comment

Stuff looks good. Still unsure about the name, may fooExtname is better because it's closer to the c/fs functions. I'll collect some feedback here

}

set(value: URI) {
this._resourceKey.set(value);
this._schemeKey.set(value && value.scheme);
this._filenameKey.set(value && basename(value.fsPath));
this._langIdKey.set(value && this._modeService.getModeIdByFilenameOrFirstLine(value.fsPath));
this._extensionKey.set(value && paths.extname(value.fsPath));
}

reset(): void {

This comment has been minimized.

@jrieken

jrieken Sep 28, 2017
Member

I'd say that is bug cc @bpasero

@jrieken
Copy link
Member

@jrieken jrieken commented Sep 28, 2017

re #34889 (comment)

@Krzysztof-Cieslak Make sure to invoke "Format Document" before pushing. We run a formatter as part of hygiene and TypeScript formatting much match, whitespace rules must be happy, and the copyright statement must be correct

@@ -28,6 +28,7 @@ export namespace EditorContextKeys {

// -- mode context keys
export const languageId = new RawContextKey<string>('editorLangId', undefined);
export const editorExtension = new RawContextKey<string>('editorExtension', undefined);

This comment has been minimized.

@jrieken

jrieken Oct 2, 2017
Member

After a second look: we actually don't need to repeat this. Having resourceExtname/Extension is enough has it gets inherited to the editor from its column/slot. The reason for having editorLangId and resourceLangId is that latter is derived from the name while the former is more correct (look at the name and the contents)

@@ -23,11 +23,13 @@ export class ResourceContextKey implements IContextKey<URI> {
static Filename = new RawContextKey<string>('resourceFilename', undefined);
static LangId = new RawContextKey<string>('resourceLangId', undefined);
static Resource = new RawContextKey<URI>('resource', undefined);
static Extension = new RawContextKey<string>('resourceExtension', undefined);

This comment has been minimized.

@jrieken

jrieken Oct 2, 2017
Member

So, I'd prefer resourceExtname. What do you think?

}

set(value: URI) {
this._resourceKey.set(value);
this._schemeKey.set(value && value.scheme);
this._filenameKey.set(value && basename(value.fsPath));
this._langIdKey.set(value && this._modeService.getModeIdByFilenameOrFirstLine(value.fsPath));
this._extensionKey.set(value && paths.extname(value.fsPath));

This comment has been minimized.

@jrieken

jrieken Oct 2, 2017
Member

Way to go but we to think if folks expect .do from foo.do or do...

This comment has been minimized.

@Krzysztof-Cieslak

Krzysztof-Cieslak Oct 6, 2017
Author Contributor

I think typical behavior is to return extension with the dot. At least that's what most standard libraries I know do - Node's path.extname, .Net Framework Path.GetExtension, or Ruby's File.extname

This comment has been minimized.

@Krzysztof-Cieslak

Krzysztof-Cieslak Oct 6, 2017
Author Contributor

More interesting question is if users expects it to be normalized to lower case (i.e if "ABC.FSX" should return ".FSX" or just ".fsx")

This comment has been minimized.

@jrieken

jrieken Oct 6, 2017
Member

Good questions... I'd say we have it "as is". I can see how a "normalized" version can help but that applies to many values, e.g. filename

@Krzysztof-Cieslak
Copy link
Contributor Author

@Krzysztof-Cieslak Krzysztof-Cieslak commented Oct 6, 2017

OK, removed new context from the ediotrContextKeys, leaving only resource one. And I've renamed the resource on to resourceExtname

@jrieken jrieken merged commit 4a1c847 into microsoft:master Oct 10, 2017
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
@jrieken
Copy link
Member

@jrieken jrieken commented Oct 10, 2017

@egamma egamma mentioned this pull request Oct 10, 2017
38 of 40 tasks complete
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.