SublimeText / sublime_lib

Utility library for frequently used functionality in Sublime Text and convenience functions or classes
https://sublimetext.github.io/sublime_lib
MIT License
52 stars 4 forks source link

Easier/better settings chaining #126

Open Thom1729 opened 5 years ago

Thom1729 commented 5 years ago

Inspired by this forum thread.

Some packages want to allow package settings to be overridden per view, per window, and/or per project. We recommend using a ChainMap of SettingsDicts for this. However, this only works if all of the Settings objects use the same keys. In practice, packages should and often do cordon off the package-specific parts of view, window, and project settings. This is usually done in one of two ways:

Even with SettingsDict and ChainMap, this requires boilerplate to implement. Consider the following code sample from ESLint-Formatter:

  @staticmethod
  def get_pref(key):
    global_settings = sublime.load_settings(SETTINGS_FILE)
    value = global_settings.get(key)

    # Load active project settings
    project_settings = sublime.active_window().active_view().settings()

    # Overwrite global config value if it's defined
    if project_settings.has(PROJECT_NAME):
      value = project_settings.get(PROJECT_NAME).get(key, value)

    return value

This only handles two levels (package and project), is read-only, and relies on active_window(). but it's still a fair bit of code. Adding window and view settings would add yet more complication.

It would seem beneficial for sublime_lib to provide an abstraction for this pattern that is robust, full-featured, and easy to use. The simplest approach (in terms of implementation) would be to define simple dictionary decorators and rely on ChainMap:

settings = ChainMap(
    PrefixedDict(SettingsDict(view.settings()), 'MySettings-'),
    PrefixedDict(SettingsDict(window.settings()), 'MySettings-'),
    NamedSettingsDict('MySettings')
)

This is still a fair bit of boilerplate, though. In addition, the ChainMap is missing the subscribe functionality that SettingsDict provides; a plugin would have to subscribe to each individually and manually check whether the value has changed.

A more usable solution would be a single constructor:

settings = SettingsChain('MySettings', window)
settings = SettingsChain('MySettings', view) # includes window implicitly?

This is a bit more opinionated than the user building their own stack piece-by-piece, but it's a lot more concise and would encourage consistency among packages that used it.

Questions:

FichteFoll commented 5 years ago

Does this generally sound like a reasonable feature to provide?

Let's start by listing the locations one can use for configuration.

  1. custom .sublime-settings file
  2. main Preferences chain (usually with a prefix)
  3. project data

I'm going to base this on best practices and not what is out there in the wild, because some setups are just too obscure.

For project data, I believe project-specific settings are the way to go and not an arbitrary subkey in the project data mapping. Thus, we'd only have to worry about the default chain. In the default chain, it is (or should be) common to prefix your settings with the package name and a dot. A PrefixedDict definitely sounds useful for that. Another option is having a mapping under a package-named key, e.g. "PackageDev": {/*settings be here*/}. That's generally not a good idea however because of how settings files are merged and I'd just not consider it. Also, you can easily use a chain map with the extracted mapping.

Considering this, we'd have the following code:

settings = ChainMap(
    PrefixedDict(SettingsDict(view.settings()), "PackageDev."),
    NamedSettingsDict("PackageDev"),
)

I believe that is a reasonable amount of boiler plate for the flexibility it provides. Should authors be using different setting locations, they can add them in where they want.

Adding a shortcut for this via SettingsChain seems superfluous at first. However, through doing so we are advocating opinionated best practices and I like that. The downside is that named settings object can't be cached like this and we'd be creating quite a few objects for every view and whenever we need this settings map. Some of that could probably be solved through keyword arguments to the constructor, but we need to be vary of feature creep there. I'd like the shortcut to be as simple as possible.

What's the right balance of convenience without confusion?

I believe the solution you suggested, i.e. creating a ChainMap based on the package name, is reasonable and not confusing. The code is self-documenting.

How should we handle modifying the chain?

Don't do anything special, imo. If the plugin wants to do modifications at all (which is rare enough), let it do so explicitly. Otherwise we just write to the view settings.

What do we do about project data?

Like I mentioned, either include them automatically through the default chain or have the user specify a subdict directly.