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

Adding VSCode setting classes #70

Closed
wants to merge 1 commit into from

Conversation

Gijsreyn
Copy link
Contributor


This PR addresses #69

@ryfu-msft
Copy link
Contributor

So I'm sure this was a lot of work, but I'm not going to review a file that large in one PR. Please separate these resources out into smaller PRs so that it is easier for me to review and provide feedback.

Also, just looking at this briefly, it seems like the settings are controlled by settings files? Do we really need a DSC resource for each setting that exists? Why can't we dynamically generate the content for any settings files and not have to specify every single property. What if properties are added or removed? Our solution should be easily maintainable as VSCode evolves.

@Gijsreyn
Copy link
Contributor Author

I thought you are a PR warrior Ryan. Just kidding, I'm messing with you. I agree with the points you've just mentioned.

The thing is, it wasn't that much of work. The most work was getting the actual first setting to work. Then it was pretty easy to script it out. To be fair, it's better illustrated with a video:

generateclasses.mp4

If you're asking me why we need separate resources, is because of the groupings on all of them. Each group has it's own unique settings and some might even overlap. That's why I said, it covers roughly 80 - 85% of the settings.

I asked Justin Grote if there's a possibility to fetch settings from a different location, but currently, there isn't.

The script in such sense, dynamically fetches all the content to make it more maintainable. It wasn't 100% fully finished, but did quite the trick.

Please abandon this pull request. Thank you.

@ryfu-msft
Copy link
Contributor

I thought you are a PR warrior Ryan. Just kidding, I'm messing with you. I agree with the points you've just mentioned.

The thing is, it wasn't that much of work. The most work was getting the actual first setting to work. Then it was pretty easy to script it out. To be fair, it's better illustrated with a video:

generateclasses.mp4
If you're asking me why we need separate resources, is because of the groupings on all of them. Each group has it's own unique settings and some might even overlap. That's why I said, it covers roughly 80 - 85% of the settings.

I asked Justin Grote if there's a possibility to fetch settings from a different location, but currently, there isn't.

The script in such sense, dynamically fetches all the content to make it more maintainable. It wasn't 100% fully finished, but did quite the trick.

Please abandon this pull request. Thank you.

Haha I wish I was a PR warrior... I promise I'll get to your dotnet resource today 😅

This is a very neat that you are able to capture almost all of the settings from a script that you made. I didn't realize how many VSCode settings there were so I'll need to do some work on my end to experiment with those settings. Regardless, thanks for creating this.

@ryfu-msft ryfu-msft closed this Oct 16, 2024
@Gijsreyn
Copy link
Contributor Author

Thanks Ryan, I'll put it on the gallery as hobby project for folks that love to use DSC for VSCode.

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