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

Would be great if multiline strings were categorized differently #2005

Closed
basarat opened this issue Feb 11, 2015 · 11 comments
Closed

Would be great if multiline strings were categorized differently #2005

basarat opened this issue Feb 11, 2015 · 11 comments
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@basarat
Copy link
Contributor

basarat commented Feb 11, 2015

As it stands, the default classification looks horrible

image

I would be happy if there was just InMultiLineString (notice I don't say Literal) + it gets classified as MultilineString

Current:

    const enum EndOfLineState {
        Start = 0,
        InMultiLineCommentTrivia = 1,
        InSingleQuoteStringLiteral = 2,
        InDoubleQuoteStringLiteral = 3,
    }
    enum TokenClass {
        Punctuation = 0,
        Keyword = 1,
        Operator = 2,
        Comment = 3,
        Whitespace = 4,
        Identifier = 5,
        NumberLiteral = 6,
        StringLiteral = 7,
        RegExpLiteral = 8,
    }

Note: feel free to call it something other than MultiLineString but give us something.

@DanielRosenwasser
Copy link
Member

#1744 has a fix for this.

@basarat
Copy link
Contributor Author

basarat commented Feb 11, 2015

@DanielRosenwasser perhaps you can quickly tell me what classifyKeywordsInGenerics is supposed to do. Just a hint on if I should keep it true or false

@basarat
Copy link
Contributor Author

basarat commented Feb 11, 2015

@DanielRosenwasser also is there some public API endpoint to get the syntacticClassifier?

Nevermind, got the answer: #1477 (comment)

@DanielRosenwasser
Copy link
Member

@basarat if you'll see my change, it's been renamed to syntacticClassifierAbsent.

Before the syntactic classifier was a thing, the lexical classifier had to do a bunch of hacky sort of tricks to try to emulate correct behavior.

Basically, you don't want to accidentally classify number as a keyword as a type parameter when it might just be a plain identifier after a less-than token. The syntactic classifier will overwrite this to be the correct thing (i.e. an identifier) and so you'll get flashing. However, if there is no syntactic classifier, the lexical classifier should use its awful tricks again.

Actually, you're using web workers for Atom right? Are you using the other classifiers?

@basarat
Copy link
Contributor Author

basarat commented Feb 11, 2015

you're using web workers for Atom right?

Nope. A child process : https://github.com/TypeStrong/atom-typescript/blob/master/CONTRIBUTING.md#architecture . Node-webworker's need C/C++ toolchain on the client to install so I just went with a child process to make it easier to install.

Are you using the other classifiers?

nope. Just a lexical classifier : https://github.com/TypeStrong/atom-typescript/blob/master/lib/main/atom/typescriptGrammar.ts#L47, so I'll make it true. Thanks :)

@danquirk danquirk added the Suggestion An idea for TypeScript label Feb 11, 2015
@basarat
Copy link
Contributor Author

basarat commented Feb 11, 2015

Reason for not using webworkers : electron/electron#797

@DanielRosenwasser
Copy link
Member

I just realized, you're asking for a multiline string classification. What if it's just a string literal classification but it's done correctly?

@basarat
Copy link
Contributor Author

basarat commented Feb 11, 2015

In a single line it gets identitified as an identifier :

image

In mutliple line it gets identified as code as there is no finalLexState for stuff with "`"

image

So for both of these I would like "just a string literal classification but it's done correctly".

@basarat
Copy link
Contributor Author

basarat commented Feb 12, 2015

@DanielRosenwasser fixed? Or wont-fix ❤️

@DanielRosenwasser
Copy link
Member

If we do this I don't have to buy you chocolates for Valentine's Day. ❤️

(Will fix, but unfortunately just for 1.5: #2026. I guess I'll reopen just to track this as an issue.)

@DanielRosenwasser
Copy link
Member

The fix is in master.

@DanielRosenwasser DanielRosenwasser added the Fixed A PR has been merged for this issue label Feb 13, 2015
@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

3 participants