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

Fix declarations of supported file extensions and filenames. #501

Merged
merged 2 commits into from
Dec 11, 2016

Conversation

JimiC
Copy link
Member

@JimiC JimiC commented Dec 11, 2016

Writing some more tests revealed some bugs we have in supportedExtensions.

@@ -96,9 +96,9 @@ exports.extensions = {
{ icon: 'image', extensions: ['jpeg', 'jpg', 'gif', 'png', 'bmp', 'ico'] },
{ icon: 'ionic', extensions: ['ionic.project'], filename: true },
{ icon: 'ionic', extensions: ['ionic.config.json'], filename: true },
{ icon: 'jade', extensions: ['jade', 'pug', 'tag'] },
Copy link
Member

Choose a reason for hiding this comment

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

why do you removed 'tag'?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's already declared in riot and creates a conflict.

Copy link
Member

Choose a reason for hiding this comment

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

Okey.

Copy link
Member

Choose a reason for hiding this comment

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

Extensions that collide should be studied. Some can be solved using languageIds. @JimiC you removed prolog language in another PR, which is not right, as languageIds can be added via plugins. In the case of Prolog you can find the extension here and if you see their contribution point you'll see that the id of prolog was correct.

Copy link
Member Author

@JimiC JimiC Dec 11, 2016

Choose a reason for hiding this comment

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

Right! Restoring and commit again.

Copy link
Member Author

Choose a reason for hiding this comment

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

Regarding the tag extension, I can't find anything to justify its presence in two places.

Copy link
Member

Choose a reason for hiding this comment

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

Cool. Yes, I only see a Riot extension. Any language extension for Jade/Pug... so let's removed as you originally intended. 👍

@JimiC
Copy link
Member Author

JimiC commented Dec 11, 2016

@robertohuertasm Shouldn't the extensions marked with language support not be present in the fileNames or fileExtensions?

@JimiC JimiC mentioned this pull request Dec 11, 2016
{ icon: 'ng2_service_js', extensions: ['.service.js'], svg: true },
{ icon: 'ng2_module_ts', extensions: ['.module.ts'], svg: true },
{ icon: 'ng2_module_js', extensions: ['.module.js'], svg: true },
{ icon: 'ng2_component_ts', extensions: ['.component.ts'], filename: true, svg: true },
Copy link
Member

Choose a reason for hiding this comment

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

Why the filename property if this is just an extension?

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems you are correct. The test was complaining... and head was buzzing. Will revise.

Copy link
Member

Choose a reason for hiding this comment

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

Ok! 👍

@@ -96,9 +96,9 @@ exports.extensions = {
{ icon: 'image', extensions: ['jpeg', 'jpg', 'gif', 'png', 'bmp', 'ico'] },
{ icon: 'ionic', extensions: ['ionic.project'], filename: true },
{ icon: 'ionic', extensions: ['ionic.config.json'], filename: true },
{ icon: 'jade', extensions: ['jade', 'pug', 'tag'] },
Copy link
Member

Choose a reason for hiding this comment

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

Extensions that collide should be studied. Some can be solved using languageIds. @JimiC you removed prolog language in another PR, which is not right, as languageIds can be added via plugins. In the case of Prolog you can find the extension here and if you see their contribution point you'll see that the id of prolog was correct.

@JimiC
Copy link
Member Author

JimiC commented Dec 11, 2016

Extensions for ng2 restablished. Will have to check the tests in the other PR regarding those extensions.

@robertohuertasm
Copy link
Member

robertohuertasm commented Dec 11, 2016

@JimiC, regarding the question about language support and extensions, for me it makes sense to have both or at least support both cases, that way if the language support doesn't support (redundance here, sorry) some of the extensions, we don't have to wait or beg the author to implement them in their language extension.

Ideally, we shouldn't have to need this duplicity but this is not always perfect so that's why I kept the dual functionality.

@JimiC
Copy link
Member Author

JimiC commented Dec 11, 2016

@robertohuertasm Yeah, I figured that out after doing some tests, but forgot to comment on my comment. Sorry.

@JimiC
Copy link
Member Author

JimiC commented Dec 11, 2016

If this PR is OK you can merge it. I'm going to adjust #502 accordingly.

@robertohuertasm robertohuertasm merged commit 2a321f9 into vscode-icons:master Dec 11, 2016
@JimiC JimiC deleted the bug_fix branch December 11, 2016 18:54
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

3 participants