SublimeText / PackageDev

Tools to ease the creation of snippets, syntax definitions, etc. for Sublime Text.
MIT License
436 stars 83 forks source link

Update 'Sublime Text Settings' package #113

Closed deathaxe closed 7 years ago

deathaxe commented 7 years ago

Here is the content of my EasySettings fork as proposal to be added to PackageDev

In the end I think it is better to have one package with most complete support for all of Sublime Text's files instead of creating more and more tiny packages which a user needs to learn about and which might interfere with each other.

FichteFoll commented 7 years ago

Thanks for your work on this. I'll probably take a few days to review & integrate this.

In the mean time, why don't you join us on the Sublime Text Discord server? Many package developers are active there and it's a great place for live discussion, unlike the forum: https://discord.gg/HcmwdVK

FichteFoll commented 7 years ago

Merged this into the syntax/settings branch and did a couple iterations on it. I'm generally very happy with how this turned out (and how it was already in this PR). I just refined it a little, made it more accurate and less error-prone and changed a couple things. Please do review my commits, if you like to.

What's left on my agenda until I can merge this:

  1. load Preferences.sublime-settings for syntax-specific settings (in addition to possible plugin settings using the same settings file name)
  2. (blocker) have a mechanism to load comments and default values from files that are not exactly the same name because they should not be set. I mostly need this for the dummy settings that you added to Package/Sublime Text Settings/Preferences.sublime-settings, because they override dynamic default values (specifically, dpi_scale).

This isn't hard to do, so I'll likely finish this up tomorrow and pack up a new alpha release.

Thanks again for contribution. I love the direction PackageDev is taking with this.

(Also, another hint towards the discord server linked above. 😉 )

FichteFoll commented 7 years ago

Also note that I'm using a max line length of 99 for code (via .flake8) as I consider 80 to be too small.

Unsure what to do about the logger instance yet. I've done this in many of my projects and like it a lot, although I understand that single-letter variable names are frowned upon.

deathaxe commented 7 years ago

There was a hard discussion about line length in GitGutter, too, as I used longer lines, too. In the end I got used to the 80 chars and honestly find code readable best if line length is limited. It forces to avoid too heavy nested function calls. Using one more variable may improve readability much in some cases.

FichteFoll commented 7 years ago

Cherry-picked most of your comments. I didn't include some line length changes that I consider to be redundant and also didn't include the l rename.

Regarding the logger instance, the variable has to be named either l (as a shortcut) or logger, because log is not telling the whole story (could be referring to a file object representing a log). I'll post-pone this to a later stage.

Besides some caching optimization, I consider this feature to be complete, so I'm gong to merge the branch and close this PR.