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

Formatting on Enter #649

Merged
merged 98 commits into from
Feb 1, 2018
Merged

Formatting on Enter #649

merged 98 commits into from
Feb 1, 2018

Conversation

MikhailArkhipov
Copy link

Includes new unicode package so not sure about the CLA here

@DonJayamanne
Copy link

@brettcannon we have a new package here, guess this means it won't make the Jan release.

@codecov
Copy link

codecov bot commented Jan 29, 2018

Codecov Report

Merging #649 into master will increase coverage by 1.14%.
The diff coverage is 84.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #649      +/-   ##
==========================================
+ Coverage   61.44%   62.58%   +1.14%     
==========================================
  Files         233      240       +7     
  Lines       10635    11066     +431     
  Branches     1852     1998     +146     
==========================================
+ Hits         6535     6926     +391     
- Misses       4094     4132      +38     
- Partials        6        8       +2
Impacted Files Coverage Δ
src/client/providers/providerUtilities.ts 100% <100%> (ø)
src/client/language/characterStream.ts 95.58% <100%> (+3.05%) ⬆️
src/client/typeFormatters/blockFormatProvider.ts 92.85% <100%> (ø) ⬆️
...c/client/typeFormatters/codeBlockFormatProvider.ts 89.65% <100%> (ø) ⬆️
src/client/extension.ts 93.63% <100%> (+0.08%) ⬆️
src/client/providers/completionSource.ts 93.22% <100%> (-1.07%) ⬇️
src/client/language/types.ts 97.14% <100%> (+2.69%) ⬆️
src/client/language/characters.ts 62.12% <62.12%> (ø)
src/client/language/tokenizer.ts 85.47% <80.12%> (-9.72%) ⬇️
src/client/language/braceCounter.ts 81.08% <81.08%> (ø)
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d596beb...7aee5ff. Read the comment docs.

Copy link

@DonJayamanne DonJayamanne left a comment

Choose a reason for hiding this comment

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

Review not completed


public Tokenize(text: string, start?: number, length?: number): ITextRangeCollection<IToken> {
public tokenize(text: string, start?: number, length?: number, mode?: TokenizerMode): ITextRangeCollection<IToken> {
Copy link

@DonJayamanne DonJayamanne Jan 29, 2018

Choose a reason for hiding this comment

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

This method is getting busier (i.e. its doing a lot synchronously).
Should we try to put an intermittently push control back to the event loop (e.g. await on a timeout or similar) to ensure VS Code has time to catch up on any UI events, which could potentially cancel this operation.

I.e. if we have a large file and this takes 1-2 seconds, then VS Code will hang until then.
We could pass in a cancellation token into this method and every few milliseconds or so we could check the value of this token (ensuring we pas control back to the main event loop).

Copy link
Author

@MikhailArkhipov MikhailArkhipov Jan 29, 2018

Choose a reason for hiding this comment

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

Tokenization, although usually somewhat long code, is typically pretty fast (tens of ms on 10000 lines). This one is pretty simple and only runs on very specific actions. VSC runs regex for colorization ;-)

This one primarily tokenizes comments and strings in a file and only used for detailed information for a single line.

I can measure if you wish.

Choose a reason for hiding this comment

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

I can measure if you wish.

Yes, it would be good to measure this.
Besides, we're testing this on our beefed up hardware. Lately I've been using this repo for simple testing (https://github.com/python/cpython.git)

Copy link
Author

@MikhailArkhipov MikhailArkhipov Jan 29, 2018

Choose a reason for hiding this comment

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

10-20 ms per 1000 lines full tokenization (which we don't normally do anyway).

Choose a reason for hiding this comment

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

👍

@MikhailArkhipov
Copy link
Author

Speaking of unicode package. Technically we could avoid it and just use ANSI for now. It is pretty rare for people to use Unicode-specific identifiers.

@DonJayamanne
Copy link

Speaking of unicode package. Technically we could avoid it and just use ANSI for now

Please do bring this up with @brettcannon

Copy link

@DonJayamanne DonJayamanne left a comment

Choose a reason for hiding this comment

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

All good, but not approving this due to the inclusion of the new package.

@brettcannon brettcannon added this to the February 2018 milestone Jan 29, 2018
@MikhailArkhipov MikhailArkhipov merged commit b45aae3 into microsoft:master Feb 1, 2018
@MikhailArkhipov MikhailArkhipov deleted the tokens branch February 1, 2018 20:40
@lock lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants