Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

fix: unecessary program updates by removing timeout methods#1693

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
bradzacher merged 10 commits intotypescript-eslint:masterfromsheetalkamat:noTimeouts
Mar 12, 2020
Merged

fix: unecessary program updates by removing timeout methods#1693

bradzacher merged 10 commits intotypescript-eslint:masterfromsheetalkamat:noTimeouts
Mar 12, 2020

Conversation

sheetalkamat
Copy link
Contributor

This should fix unnecessary program updates that happen right away whenever something changes

@typescript-eslint
Copy link
Contributor

Thanks for the PR,@sheetalkamat!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


🙏Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently onhttps://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitorsper day.

@bradzacher
Copy link
Member

bradzacher commentedMar 7, 2020
edited
Loading

Thanks for looking into the performance for us! I really appreciate the help 😄

It looks like this change breaks the persistent parse tests, which ensure that the IDE use case is working as expected.

you can run them locally from within thetypescript-estree package:

cd packages/typescript-estreeyarn test persistentParse

@bradzacherbradzacher added the bugSomething isn't working labelMar 7, 2020
@sheetalkamat
Copy link
ContributorAuthor

cd packages/typescript-estree
yarn test persistentParse

Will do

@codecov
Copy link

codecovbot commentedMar 9, 2020
edited
Loading

Codecov Report

Merging#1693 intomaster willdecrease coverage by0.01%.
The diff coverage is81.81%.

@@            Coverage Diff             @@##           master    #1693      +/-   ##==========================================- Coverage   95.21%   95.19%   -0.02%==========================================  Files         148      148                Lines        6953     6967      +14       Branches     2006     2007       +1     ==========================================+ Hits         6620     6632      +12- Misses        123      124       +1- Partials      210      211       +1
Impacted FilesCoverage Δ
...pt-estree/src/create-program/createWatchProgram.ts87.02% <81.81%> (-0.11%)⬇️

@bradzacherbradzacher removed the awaiting responseIssues waiting for a reply from the OP or another party labelMar 10, 2020
@sheetalkamatsheetalkamat dismissed a stale review viad9a5867March 11, 2020 00:01
@sheetalkamat
Copy link
ContributorAuthor

@uniqueiniquity This PR is ready to review and merge with the changes we talked about

bradzacher reacted with thumbs up emoji

@sheetalkamat
Copy link
ContributorAuthor

And I am not sure failing codecov/patch is about

@uniqueiniquity
Copy link
Contributor

@bradzacher any thoughts on the code coverage issue? it's a preemptive change for 3.9 so it makes sense that the path isn't taken.

@bradzacher
Copy link
Member

bradzacher commentedMar 11, 2020
edited
Loading

If you want to satisfy the branch coverage, you should be able to use istanbul ignore comments to stop the coverage reporting the missed branch.

/* istanbul ignore if */if(isRunningNoTimeoutFix){// ....}/* istanbul ignore else */if(!isRunningNoTimeoutFix){// ....}

I keep it around to try and prompt contributors to cover their code appropriately, though I don't usually treat code coverage as a hard merge blocker because there's always some edge cases and checks to keep the types "safe" (unless of course the coverage is terrible, then I RC 😅).

When it fails, I usually review thecoverage report to see what code was missed, and if it's something that should be tested.
(yellow =if has a missed branch, red = code not executed)

If you're happy with this change, approve it and I'll gladly override the failed check to merge it.

(again, thanks to you both for looking into all of this for us)

sheetalkamat reacted with thumbs up emoji

@sheetalkamat
Copy link
ContributorAuthor

I think we should ignore the coverage result instead of adding ignore comment since as soon as 3.9 releases things should start working... I will update this with master for merge

bradzacher reacted with thumbs up emoji

@bradzacherbradzacher merged commit2ccd66b intotypescript-eslint:masterMar 12, 2020
@sheetalkamatsheetalkamat deleted the noTimeouts branchMarch 12, 2020 19:47
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@uniqueiniquityuniqueiniquityuniqueiniquity approved these changes

Assignees
No one assigned
Labels
bugSomething isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@sheetalkamat@bradzacher@uniqueiniquity

[8]ページ先頭

©2009-2025 Movatter.jp