Skip to content

[fix] Wrongly tagging PascalCase symbols as types #290

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

Closed
wants to merge 1 commit into from

Conversation

carlos-algms
Copy link

@carlos-algms carlos-algms commented Apr 17, 2024

Reasoning

Enums, Class names, constants, and anything using PascalCase was tagged as Type, which in most cases is not the case.

Typescript's documentation suggests that things like Enums should be PascalCase, and it's how they write their docs examples and also the code that is exposed to users.
There are also some cases, where a const can be used to replace an enum, like:

const Priorities = {
  Max: 1000,
  Mid: 500,
  low: 100,
};

image

For the other cases, like type and interface, they are already covered the type_identifier on line 3.

image

Checklist

  • All tests pass in CI.
  • There are sufficient tests for the new fix/feature.
  • Grammar rules have not been renamed unless absolutely necessary.
  • The conflicts section hasn't grown too much.
  • The parser size hasn't grown too much (check the value of STATE_COUNT in src/parser.c).

@maxbrunsfeld
Copy link
Contributor

I think this rule is present in order to achieve consistent highlighting in code like this:

class ThisIsAType {
  // ...
}

const a = new ThisIsAType;

Typescript's documentation suggests that things like Enums should be PascalCase

In TypeScript, enums are types, so I think it is reasonable to highlight them as types.

There are also some cases, where a const can be used to replace an enum,

While not all PascalCase names are types, I do think that the vast majority of types are PascalCase by convention. Without this convention-based rule, there would be a lot of type names that are highlighted inconsistently between different usage sites.

@carlos-algms
Copy link
Author

carlos-algms commented Apr 18, 2024

@maxbrunsfeld
In your example, you have 2 things that are not types:

  1. class ThisIsAType: which is a class, and is represented by (class_declaration....)
  2. new ThisIsAType: which is class instantiation, and also has the existing selector: (new_expression)

Types are symbols usually present after a colon : .... as in:

const name: string = '...';
const isOdd = (): boolean => {....}
function sum(a: number, b:number): number {}

Or, when explicitly defining interfaces and type aliases:

type Union = User | Admin;

interface Duck extends Animal {}

And, yes, enums are considered types, when used as types:

// This is covered by the selector: (enum_declaration)
enum Level {
  Log,
  Error,
}
//                         | This should be flag as type
function setLevel(level: Level) {};

@maxbrunsfeld
Copy link
Contributor

maxbrunsfeld commented Apr 18, 2024

In your example, you have 2 things that are not types

No. A class is both a value and a type, and we highlight it as a type very intentionally.

I'm not clear on whether you're understanding my point though. I'll just summarize one more way. An important goal with these highlight queries is to highlight a given entity the same in all of the places that it occurs. As long as we abide by that constraint, we want to provide as specific of a categorization as possible. Because of the nature of TypeScript, there are many entities that can be used both in value and in type position.

class T {
}

const a: T = buildT();

T.someMethod();

We don't want T to show up in several different colors here.

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