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

[Common] Adjust wildcard scopes #3803

Merged
merged 12 commits into from
Aug 2, 2023
Merged

Conversation

deathaxe
Copy link
Collaborator

@deathaxe deathaxe commented Jul 8, 2023

This PR implements RFC #3766 by ...

  1. adjusting path wildcard scopes to variable.language.wildcard
  2. modifying scopes of . to variable.langauge.self
  3. modifying scopes of .. to variable.langauge.parent

The current scopes are chosen ...

  1. to fullfill requirements outlined in [RFC] Path wildcard scope #3766 (comment)

  2. because variable.language.wildcard has been used by majority of syntaxes for quite a while

  3. as they fit into a common naming scheme

    variable
       language
          anonymous      - empty throwaway variables (e.g. `_`)
          self           - `self`, `this`, `.`, ...
          parent         - `super`, `..`, `&` in CSS, ...
          wildcard       - `*`, `?`, ...
    

    Those scopes may be used by path like tokens such as path strings or qualified identifiers as meta.path is.

Note: It partly reverts #3768 and #3769.

Alternative

An alternative would be to move them all under the constant.other.placeholder scope if variable feels too odd within string constants, but this may require scope modifications to placeholders of format strings to make them distinguishalbe. The idea would be to

constant
  other
    placeholder
      format
      parent
      self
      wildcard
        asterisk
        questionmark

Disadvantages:

  1. Scope names become longer
  2. Can't re-use existing self/parent scopes
  3. More syntaxes would be effected/need to be changed

EDIT 2023/07/10

This PR finally follows proposed scope naming of #3803 (comment)

constant
  other
    path
      parent
      self
    wildcard
      asterisk
      questionmark

This commit puts `.` and `..` into same scope category as `*` or `?` wildcards
by scoping 

| token  | scope
+ ---    + ---
| `.`    | `variable.language.self`
| `..`   | `variable.language.parent`

Those scopes are also used by keywords such as `self`, `this`, `super` in other
contexts, where they denote path like access.

Note:
- `variable.language` is chosen as most syntaxes have been using
  `variable.language.wildcard` for quite a while, so it's probably not a good
  idea changing it. Same applies to path scopes changed by this commit, but
  those are not so many.

The big picture:

```
variable
   language
      anonymous      - empty throwaway variables (e.g. `_`)
      self           - `self`, `this`, `.`, ...
      parent         - `super`, `..`, `&` in CSS, ...
      wildcard       - `*`, `?`, ...
```
@michaelblyons
Copy link
Collaborator

I am okay with this. I support having consistency.

I'm very slightly disappointed that we lose the distinguishing between * in file patterns that is next to a / and one that is not. Maybe the / could be included in the match? I forget the custom globbing behavior all the time, and I honestly made the file pattern syntax just as a reminder.

@deathaxe
Copy link
Collaborator Author

deathaxe commented Jul 9, 2023

Do I understand correctly, the distinguished scopes' purpose is to express the different behavior/meaning of * depending on whether / is present in a path as desribed at https://www.sublimetext.com/docs/file_patterns.html#path-rules?

So that distinction is a very special case related with ST's way to handle file patterns?

Wouldit be ok to distinguish them via another sub-scope such as variable.language.wildcard.asterisk.path vs. variable.language.wildcard.asterisk..file?

ST treats `*` depending on whether being used in a path with `/` or in simple
file patterns. This commit re-introduces distinct scopes based on that, to
enable them being highlighted differently.

Note:

The old and resurrected behavior does not yet apply expected scope to patterns
like `fo*der/`. A fix is however out of scope for this PR.
FichteFoll
FichteFoll previously approved these changes Jul 9, 2023
Copy link
Collaborator

@FichteFoll FichteFoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yet another example why I'm a little sad that we always squash PRs when merging because reviewing this by commit was most likely the much better experience, being able to correlate test changes to syntax changes et al.

Batch File/Batch File.sublime-syntax Outdated Show resolved Hide resolved
@michaelblyons
Copy link
Collaborator

...purpose is to express the different behavior/meaning of * depending on whether / is present in a path as desribed at https://www.sublimetext.com/docs/file_patterns.html#path-rules?

So that distinction is a very special case related with ST's way to handle file patterns?

Yes, that was the aim. Thanks.

michaelblyons
michaelblyons previously approved these changes Jul 9, 2023
@jwortmann
Copy link
Contributor

The good:
This PR improves consistency between syntaxes by unifying the wildcard symbols under a single top-level scope. And the scopes are specific enough to enable color schemes to target them precisely. So overall this is a good improvement, compared to current situation.

The bad:
I think making use of the variable.language here goes against to what was the intended usage according to the scope naming guidelines for this scope:

Reserved variable names that are specified by the language, such as this, self, super, etc. should use:

  • variable.language

To me this very much reads like it was meant to distinguish special identifier names from other user-defined identifiers, which are variable.other. The wildcard symbols from this PR cannot be used as user variable names per se, because they are not (alphanumeric) "word"-characters. Btw, the keyword.operator.wildcard used by some syntaxes was also not really correct from the semantic meaning in my opinion, because operators are applied to one or more operands, but here those are just some special wildcard symbols which can more or less stand alone and are not applied to some operands.

Instead, I would go with the alternative of constant.other, because there is precedence in the scope naming guidelines with constant.other.placeholder. But I would adjust the suggested scheme a bit and use different 3rd-level subscopes, so that there is no need to refine the existing constant.other.placeholder scope used for string placeholder symbols:

constant
  other
    placeholder
    wildcard
      asterisk
      questionmark
    path
      self
      parent

Yeah, you could argue that the placeholder symbols feel more like variables than constants, but then you could say the same for the existing constant.other.placeholder scope in strings. So I would suggest to go with the existing scheme and extend it to the other wildcard/placeholder symbols.

The ugly:
If you really want to go with variable.language, then at least some of the 3rd and 4th-level subscopes are mixed up right now. It should be variable.language.path.{self|parent} instead of the other way round. There is the "re-use existing self/parent scopes" mentioned as it were an advantage, but in my opinion this would actually be a disadvantage. Because the more important property of the . and .. placeholders is that they describe some filepath, and not that they were some specialization of (e.g. Python's) self and super keywords.

@deathaxe
Copy link
Collaborator Author

deathaxe commented Jul 9, 2023

I second concerns about variable vs. constant and critism on keyword.operator.wildcard. Those are part of having been struggling to find a suitable scope for those tokens. Applying variable scope to parts of a literl string doesn't feel perfectly well, with the format placeholders in mind, but I justified it (for myself) with

  1. interpreting it like a kind of interpolation. Wildcards differ from format strings by not representing a placeholder to be replaced by a given constant or variable.
  2. visual distinction between wildcards and placeholders working reliably even with very simple or legacy color schemes.
  3. it being more commonly used in majority of syntaxes already.

I have no strong oppinion about variable vs. constant. I'd find both more or less equally suitable. I'd be happy to change scopes even though it would effect some more syntaxes. It would resolve "The Ugly" part as well.

Any other oppinions about it?


It should be variable.language.path.{self|parent} instead of the other way round.

It might not be optimal to re-use those scopes, but I just didn't want to introduce a variable.language.path.[parent|self] when there's already a variable.language.[self|parent] as it might not be obvious which one to use in which situation.

In general it's a question of perspective, which is the more general or specific part of a scope and there's probably no ultimatively correct answer. Therefore let's take punctuation.separator.slash.path vs. punctuation.separator.path.slash as an example.

Version A:

Perspective/Goal: All slash tokens should be grouped under a scope.

Result: The 1st scope punctuation.separator.slash.[path|arguments|parameters|...] would be choosen with slash being the more general and thus left-most scope.

Version B:

Perspective/Goal: All path-like separators what kind they ever might have should be grouped.

Result: The 2nd scope punctuation.separator.path.[slash|dot|...] would be choosen with path being the more general and thus left-most scope.

Conclusion:

Just needs an aggreement about the perspective(s) to use.

The more general problem is probably path being a semantic scope and slash a syntactic one. So the question would be priorize semantic or syntactic scoping?

Both have pros and cons, while semantic highlighting tends to result in more complex syntax definitions and even not being able to be aplied everywhere. It's just not possible with some synaxes/languages.

@FichteFoll
Copy link
Collaborator

I have no strong oppinion about variable vs. constant. I'd find both more or less equally suitable. I'd be happy to change scopes even though it would effect some more syntaxes. It would resolve "The Ugly" part as well.

Same for me.

@michaelblyons
Copy link
Collaborator

michaelblyons commented Jul 9, 2023

I am embarrassed to admit that my original scheme with some keyword.operator and some variable.language was influenced by Mariana default colors.

If the scope settles on constant.placeholder, scheme designers may want to take note and differentiate from, say, constant.character.

But the scope choice should be independent of color choice.

@jwortmann
Copy link
Contributor

I don't really think the punctuation.separator.slash.path example is a good comparison to this one. In the slash example, that scope is a direct name of the character, i.e. it is indeed a syntactic/visual description. So there is really the question of whether to give precedence of highlighting all slashes the same, or to make it easier to target all path-like separators.

But here for path vs. self/parent, both scopes only carry a semantic meaning. And I doubt that someone would prefer to highlight them in the way that . and self/this use a certain color A, but .. and super both use a different color B. Instead, I think it would be expected and much more common that self/this and super use the same color, and . and .. use a common color, which is possibly different from self/this/super. This is what I'm used to for many years and in many (all) editors & websites. For example, it should be a valid assumption that color schemes don't want to give a special color to ./.. in strings, because they are not really important to the user (unlike the %s etc. placeholders). So with variable.language.path (or with constant.other.path), it would only need to target a single scope, while with variable.language.{self|parent}.path it needs to list both of the scopes individually.

Or in other words, if you consider the set of tokens {., .., self, super} and need to devide them into two separate groups (for the purpose of highlighting in mind), then I would say it is much more natural that {., ..} belong together and also {self, super}.


Regarding variable vs. constant; I'm under the impression that the variable scope is supposed to be used for identifiers that in general can act like variables in the programming language sense, i.e. names to which you as a user can (usually) assign values to (e.g. function names, normal variable names, also _ fits here as variable.language.anonymous). And the constant scope is more often used for other things with special behavior in the particular programming language, e.g. the constant.character.escape describes just the special predefined string characters, but you cannot assign other values to it. In a similar way, you cannot directly assign values to . in paths or * in imports. They are resolved automatically.

@FichteFoll
Copy link
Collaborator

FichteFoll commented Jul 9, 2023

Instead, I think it would be expected and much more common that self/this and super use the same color, and . and .. use a common color, which is possibly different from self/this/super.

I definitely understand this point and was coming into this PR more with the question whether relative path indicators should be specially highlighted at all since those (virtual) paths actually exist for almost every tooling that interacts with the filesystem because it's implemented at such a low level that you can't really claim that it is a feature of Bash (or any other shell). I mean, we don't highlight these for strings in Python either. Because the PR was only trying to apply the results of the RFC discussion onto the default syntaxes and I didn't take part in the RFC discussion, I didn't want to bring it up, but since it appears to be a relevant point of this discussion now, I'm raising it now.

(I also really hate the "other" part in scope names, which was a really bad idea back in the days and artificially increases the length of scope names with no benefit. But that's just off-topic.)

@deathaxe deathaxe dismissed stale reviews from michaelblyons and FichteFoll via 061c79f July 10, 2023 16:19
@deathaxe
Copy link
Collaborator Author

Let's see how long it takes for users to complain about color changes in Mariana. 😄

@deathaxe deathaxe requested a review from FichteFoll July 30, 2023 12:24
@deathaxe deathaxe merged commit 6160ce5 into sublimehq:master Aug 2, 2023
@deathaxe deathaxe deleted the pr/wildcard-vars branch August 2, 2023 18:34
FichteFoll added a commit to SublimeText/PackageDev that referenced this pull request Sep 11, 2023
Has been discussed in an RFC and default packages PR.

See-also: sublimehq/Packages#3803 (comment)
See-also: sublimehq/Packages#3766
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.

5 participants