SublimeText / TrailingSpaces

Highlight trailing spaces and delete them in a flash.
MIT License
896 stars 94 forks source link

Clean up settings #140

Closed rchl closed 4 years ago

rchl commented 4 years ago
FichteFoll commented 4 years ago

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

FichteFoll commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.

rchl commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

Some (references for) inspiration:

FichteFoll commented 4 years ago

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 commented 4 years ago

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.