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

Expose configuration key for semicolon occurrence #35

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

felicio
Copy link

@felicio felicio commented Oct 30, 2017

Very much work-in-progress, but I'm hoping this will steer down a conversation about future form of this PR, whose attempt is to provide the user with an option of opting out of resolving snippets only with semicolons.

I guess the main question is: Is this request welcomed?

Resolves #27

@felicio
Copy link
Author

felicio commented Nov 4, 2017

@xabikos, I'm very curious what do you think about the direction this PR is taking. Please, let me know if I can be of any help on the maintainer side.

@felicio felicio changed the title [WIP] Expose configuration key for semicolon occurrence Expose configuration key for semicolon occurrence Nov 4, 2017
@felicio felicio force-pushed the master branch 3 times, most recently from 5a44626 to 295c398 Compare November 5, 2017 20:18
@xabikos
Copy link
Owner

xabikos commented Nov 13, 2017

@felicio My apologies for the late reply.

Thanks for the contribution and the PR.

I haven't been involved on developing a real extension for VS Code but if I understand correctly the PR changes entirely the nature of the package and from simple snippets transforms it to proper extension for the editor.

This requires to have an extra setting to specify if the snippets should include a semicolon or not, is this correct. We could provide a default value for that and then give the user the option to change it, right?

I like the idea to be honest, as I see more and more people don't use semicolons any more.

@felicio
Copy link
Author

felicio commented Nov 18, 2017

That's correct. You did the harder part, honestly.

It does that by resolving the snippets on-the-fly while responding to a change in configuration property called snippets-javascript.semi, by default set to true. Obviously, I like the idea too, however there are two things about this particular solution that I'd like to point out.

  • Minimum supported VS Code version becomes 1.8.0 (current stable is 1.18.0).
  • Snippets are maintained as an object instead of JSON.

Note: To see it in action one can fetch this branch, open in it in VS Code and Debug > Start Debugging.

@xabikos
Copy link
Owner

xabikos commented Nov 19, 2017

@felicio thanks for the explanation and the additional check in that improves the extension.

I have a couple of questions though.

Regarding the setting is it going to be a regular setting that is accessible the regular VS Code settings
page (Ctrl + ,). If not we should do it in this way if possible.

Now about the VS Code engine I don't consider this a problem as you mentioned that the current version of the editor is 1.18.0. To ask as minimum version 1.8.0 is more than reasonable. In any case once we publish this the major change is going to be increased to indicate that.

I will try to spend some time and look into the code. Please let me know if you plan to push any additional changes, so to know if I can merge this. I guess that I can also create the extension locally and install in from there to test it.

@felicio
Copy link
Author

felicio commented Nov 19, 2017

is it going to be a regular setting that is accessible the regular VS Code settings
page

Yes, it is. Personally I use "snippets-javascript.semi": false.

let me know if you plan to push any additional changes

The solution is final. Let me just squash some commits, which I'll do right away.

Handles snippet resolving in memory instead of through file system I/O.

- Adds extension main module
- Bumps `vscode` engine to 1.8.0
- Explicitly specifies activation events
- Adds configuration object
- Deletes unknown 'vue' language identifier in favour of file
  associations
- Updates extension display name and description
- Formats CHANGELOG.md
@felicio
Copy link
Author

felicio commented Nov 19, 2017

Updated. I'll gladly answer any further questions here.

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.

2 participants