Skip to content

Conversation

@deathaxe
Copy link
Member

Btw. nice changes done in the recent commits. To continue completing each others ideas, this PR proposes some fixes for regressions introduced by 30983c7 which cause:

  1. duplicated completions (e.g.: "caret_style") and
  2. missing (default) marks for list values (e.g.: "indent_guide_options").

The method _completions_from_comment() needs to do default marking in order to prevent _completions_from_default() to add the same value.

The format_completion_item() function needs to handle list type default values when setting is_default.

Force _completions_from_default() to mark values added from the default list by passing is_default=True to the format_completion_item(). It's just an optimization to avoid the value in default check.

This commit fixes some regressions introduced by 30983c7
which cause

1. duplicated completions (e.g.: "caret_style") and
2. missing `(default)` marks for list values (e.g.: "indent_guide_options").

The method `_completions_from_comment()` needs to do default marking in order to prevent `_completions_from_default()` to add the same value.

The `format_completion_item()` function needs to handle `list` type default values when setting `is_default`.

Force `_completions_from_default()` to mark values added from the
`default` list by passing `is_default=True` to the `format_completion_item()`. 
It's just an optimization to avoid the `value in default` check.
@FichteFoll
Copy link
Member

FichteFoll commented Sep 20, 2019

Good catches.

The method _completions_from_comment() needs to do default marking in order to prevent _completions_from_default() to add the same value.

For some reason I thought it already did that, but seeing how it never had the default available, this is kind of silly. I'll blame it being late yesterday.

The format_completion_item() function needs to handle list type default values when setting is_default.

Totally forgot about that.

Yesterday I thought that tests for this part of PackageDev would be neat because it's easy to regress like this, but the thought of the required effort of making the code testable put a quick end to that.

Also, while we're on this topic: I also thought about checking whether we're inside a settings file that is based on Preferences.sublime-settings to check whether scheme completions should be provided, but it's not really worth the effort and this setting name is unlikly to be used for something else. Similarly for theme completions, although those are only valid inside an actual Preferences.sublime-settings file.

@FichteFoll FichteFoll merged commit 745c695 into SublimeText:master Sep 20, 2019
@FichteFoll FichteFoll added this to the 3.2.12 milestone Sep 20, 2019
@deathaxe deathaxe deleted the pr/fix-default-settings branch September 29, 2019 10:32
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.

2 participants