vscode-workspace-config-plus

vscode-workspace-config-plus copied to clipboard

As per discussed in #107, it would be good if we provide config options to control the deep merge behavior.

@calebcartwright to start a discussion here, what do you think about using https://github.com/TehShrike/deepmerge#ismergeableobject in order to add a property filter?

This way users would be able to specify which properties should be deep merged and which should not.

Second thing, I believe we should either

• support the config property only in settings.shared.json as if it would be defined elsewhere, the merging process would be highly indeterministic
• or put it in separate file (maybe vscode-workspace-config-plus.settings.json ?)

Also, an idea, but even I find it myself very non-standard and extreme - allow to configure it through JavaScript file vscode-workspace-config-plus-deepmerge-options.js that we can expect to export options object directly passed to deepmerge. On the other side, it would solve all of the issues and not force us to create an abstraction layer over deepmerge options.

Thanks for starting the thread!

IMO allowing users to configure the merge behavior for arrays is the top priority, but happy to ideate on this for a bit to try to identify the best strategic goal.

I hadn't considered using a custom configuration file/mechanism, and was originally just thinking about following the standard VS Code Extension Settings/Configuration paradigm, i.e.:

https://code.visualstudio.com/api/ux-guidelines/settings https://code.visualstudio.com/api/references/contribution-points#contributes.configuration

I think some benefits of that approach:

• Utilize VS Code's native event system to pick up updates automatically without having to attach another file watcher
• Consistent experience with other VS Code extension settings
• Ability for individuals working in a shared repository to have different behavior that best suits their respective needs

I recognize the benefit of trying to minimize the amount of work/code required to facilitate an abstraction, but as a general philosophy I prefer to avoid leaking specifics of internals (like the API of a 3rd party lib currently utilized) to end users.

I'm not vehemently opposed to any of the options you've articulated, but curious what your thoughts are on the native VS Code extensions option too

Sure, I agree that following the standard VS Code Extension Settings would be the best way to go but this way we would have to come up with rules on how we apply them in our merging process:

• do we take values directly from settings.shared.json and ignore settings.json and settings.local.json?
• do we specify different merging options per operation (different settings for settings.shared.json + settings.local.json and for (settings.shared.json + settings.local.json) + settings.json)?

I think it all gets complicated which I think we should avoid (we're trying to store extension configuration in a file that extension essentially manages).

I agree as well with you on not leaking specifics of internals. This IMO presents a separate config file as the best solution (the clearest and most straightforward). Of course, this way we're doing something that is very custom and non-standard, but I think that this feature targets mostly advanced users (or repo maintainers) for whom things like that are understandable. Also, besides configuring deep merge settings, I'm thinking about introducing more options like:

• don't merge with settings.json: only merge settings.shared.json with settings.local.json and write it directly to settings.json (however there's #109 that could be done instead of this config option)
• https://github.com/swellaby/vscode-workspace-config-plus/issues/36

Please let me know what you think about it!

As mentioned before, I can work on PR that introduces this feature.

PS. Is there any chance of releasing the extension with the previous PR?

Apologies for the delayed response

support the config property only in settings.shared.json as if it would be defined elsewhere, the merging process would be highly indeterministic

Could you elaborate here to help me better understand the concern? I get the impression there's a chicken vs. the egg scenario you're trying to avoid, but I'm not seeing how that would occur (or at least not seeing how we wouldn't be able to prevent it)

PS. Is there any chance of releasing the extension with the previous PR?

No, not as is. While we're obviously still pre-v1.0 release, I think it's a change that would cause too much churn and be detrimental to certain usage patterns, and I don't want to put that out there without a config option that allows users to control it.

fyi I've got a working change set locally that gives users control over the array merge behavior, and I'd be comfortable moving ahead with it once I finish polishing it up.

At this point I feel like that's all that's really needed. I don't think we need to go to the extreme of exposing object-level merge behavior because with deep merging in place as the default users should still be able to "unset" any shared-level settings within the property tree via an explicit local override (e.g. foo.bar.baz.qux = ''). Yes that'd be a bit verbose but they still can do so, and it's basically a one-time-only activity.

Conversely with arrays, there's really nothing users could do if our extension doesn't explicitly give them the ability to specify. That's why I think making array behavior configurable is an absolute necessity, but also why I don't think we need to go beyond that (at least not at this point/not unless a compelling use case comes forward)