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

Performance improvement (single AST traversal run) #80

Merged

Conversation

visusnet
Copy link
Contributor

This is a continuation of #77.

Please use #79 for discussion.

@visusnet visusnet force-pushed the feature/performance-refactoring-part1 branch from 06a19de to 6b19bde Compare February 21, 2020 11:05
@visusnet visusnet force-pushed the feature/performance-refactoring-part1 branch from 6b19bde to d883dd5 Compare February 21, 2020 11:05
@visusnet visusnet changed the title Added support for refactorings that share an AST traversal run. [Draft] Performance improvement (single AST traversal run) Feb 21, 2020
@visusnet visusnet changed the title [Draft] Performance improvement (single AST traversal run) Performance improvement (single AST traversal run) Feb 21, 2020
src/types.ts Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
src/refactorings/simplify-ternary/index.ts Outdated Show resolved Hide resolved
src/action-providers.ts Outdated Show resolved Hide resolved
src/action-providers.ts Outdated Show resolved Hide resolved
src/action-providers.ts Outdated Show resolved Hide resolved
src/action-providers.ts Outdated Show resolved Hide resolved
src/action-providers.ts Show resolved Hide resolved
src/action-providers.ts Show resolved Hide resolved
visusnet and others added 2 commits March 2, 2020 21:39
Co-Authored-By: Nicolas Carlo <nicolascarlo.espeon@gmail.com>
Co-Authored-By: Nicolas Carlo <nicolascarlo.espeon@gmail.com>
@visusnet
Copy link
Contributor Author

visusnet commented Mar 2, 2020

@nicoespeon I'll check the other suggestions tomorrow (March 3rd, UTC+1 timezone).

@nicoespeon
Copy link
Owner

@visusnet also, I'll be able to give you a hand on this if you need to. Just let me know ;-)

@visusnet
Copy link
Contributor Author

visusnet commented Mar 3, 2020

@nicoespeon My team and I are on a team event, i.e. I won‘t be able to do something today. I‘m not sure yet how much there is to do. I‘ll come back to your offer if necessary. Thank you. :)

@nicoespeon
Copy link
Owner

nicoespeon commented Mar 3, 2020

@visusnet no worries. I think I should be able to tackle my comments on Thursday morning (EST time) 😃

Since I agree with the direction of the PR, my goal is to get this merged while keeping master shippable 👍

Enjoy your team event and thanks for your precious help!

@nicoespeon
Copy link
Owner

Soooo, I didn't have time to work on this today. I won't have time to do so this week-end and next week neither (conference incoming). That's fine, there's no hurry 👍

@visusnet
Copy link
Contributor Author

visusnet commented Mar 6, 2020

@nicoespeon I have updated this PR with most of your suggestions. I have rebased the other PRs. If you have more suggestions, feel free to change them directly. This might be more efficient than telling me minor things which I will just click to have them committed anyway. I have given you access to my fork. Have a nice conference week and give me a heads up in case you'll be at a European (especially in Germany) conference. I feel the urge to buy you a beer.

Confirmed it works!
It's a union type issue. Path is of type `NodePath<Node>` by default,
whereas a visitor expects `NodePath<File>` sometimes.

At runtime it should be OK.
If it's not, at least it won't crash (it will fail silently).
@nicoespeon nicoespeon merged commit 2196bbe into nicoespeon:master Mar 18, 2020
@nicoespeon nicoespeon deleted the feature/performance-refactoring-part1 branch March 18, 2020 12:42
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.

None yet

2 participants