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

Clean up settings #140

Merged
merged 2 commits into from Apr 27, 2020
Merged

Clean up settings #140

merged 2 commits into from Apr 27, 2020

Conversation

rchl
Copy link
Member

@rchl rchl commented Apr 13, 2020

  • separate settings into its own class
  • apart from trailing_spaces_highlight_color which has special handling,
    get all options directly from settings so that they are always up to
    date with user preferences. Previously settings were only read on init.

- separate settings into its own class
- apart from `trailing_spaces_highlight_color` which has special handling,
  get all options directly from settings so that they are always up to
  date with user preferences. Previously settings were only read on init.
@rchl rchl requested a review from michaelblyons April 13, 2020 10:49
@FichteFoll
Copy link
Member

I suggest using https://sublimetext.github.io/sublime_lib/#sublime_lib.NamedSettingsDict and a dict with defaults with collections.ChainMap.

@FichteFoll
Copy link
Member

Actually, I also strongly suggest dropping the redundant trailing_spaces prefixes from the setting names since you're already in trailing_spaces.sublime-settings.

@rchl
Copy link
Member Author

rchl commented Apr 17, 2020

Great suggestion about using sublime lib.
As for dropping prefixes, it would be nice but is it worth to introduce such breaking change? Or are you thinking to introduce migration code also?

@FichteFoll
Copy link
Member

FichteFoll commented Apr 17, 2020

Or are you thinking to introduce migration code also?

Would be trivial, imo. Just wrap the settings dict it in a SettingsDict subclass that prefixes all keys with trailing_spaces_ and add that between the normal SettingsDict and the defaults in the chain map.

Also remove `trailing_spaces_` prefixes from settings. Settings with
prefixes are still respected for backwards compatibility.
@rchl
Copy link
Member Author

rchl commented Apr 17, 2020

Or are you thinking to introduce migration code also?

Would be trivial, imo. Just wrap the settings dict it in a SettingsDict subclass that prefixes all keys with trailing_spaces_ and add that between the normal SettingsDict and the defaults in the chain map.

That extra layer actually needed to be first in the chain.
That is because the NamedSettingsDict that handles settings without prefixes would return value of unprefixed settings as it also reads it from default settings.
But in any case, it works fine now.

@MPvHarmelen
Copy link

MPvHarmelen commented Apr 26, 2020

Hi!

TLDR: I like the fix of (re)loading settings, I don't like removing the traling_space_ prefix.

I ended up here because I would want to be able to change trimming settings between syntaxes and projects etc. and this doesn't seem possible yet.

It doesn't seem that this pull request addresses that issue, but if you were to accept this pull request, it would become unclear which setting to use: normally, Sublime's philosophy is to use one settings key that you can use anywhere (project, syntax or user settings), but leaving off the trailing_spaces_ prefix would make it unclear what the keys mean if you find them in a project or syntax settings file. (For example, what would include_current_line mean when found in Markdown.sublime-settings?)

What do you (plural) think?

@MPvHarmelen
Copy link

MPvHarmelen commented Apr 26, 2020

Some (references for) inspiration:

  • Just the plain key everywhere, like LaTeXTools. I would then strongly root for keeping the trailing_spaces_ prefix.
  • Dotted (project) settings keys like SublimeLinter, e.g. trailing_spaces.include_current_line in project or syntax settings, but include_current_line in trailing_spaces.sublime-settings.
  • An extra key like PyTest, e.g.:
    {
      "settings": {
        "trailing_spaces": {
          "include_current_line": true,
        }
      }
    }

@FichteFoll
Copy link
Member

FichteFoll commented Apr 27, 2020

This package doesn't read from computed settings currently. Maybe it should, but the feature doesn't exist at the moment. All the settings currently being read are automatically namespaced through the file they are defined in.

If reading computed settings was to be added, however, I personally (and other plugin authors as well) favor the dotted approach of SublimeLinter, as that clearly separates the namespace from the setting's name and does not suffer from the problem of subsequent overrides that a nested mapping key would (e.g. overriding only one key in a syntax-specific settings file and another in the project settings, where the override applied latest would shadow any earlier override).

@rchl
Copy link
Member Author

rchl commented Apr 27, 2020

I am up for dotted approach too.
And that doesn't conflict with this change IMO. We can keep these setting names short while in projects require them to have a dotted prefix.

@rchl rchl merged commit 820c18f into SublimeText:master Apr 27, 2020
@rchl rchl deleted the maintenace/cleanup-settings branch April 27, 2020 08:14
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