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

chore: add visitors #2

Merged
merged 9 commits into from
Jun 17, 2024
Merged

Conversation

maastrich
Copy link
Collaborator

@maastrich maastrich commented Jun 10, 2024

what does it do?

Add visitors for remaining nodes types with tests for most of them

why?

I was planning do make a similar package but you seem to have handled most of the work and that I could handle some of the rest

@maastrich
Copy link
Collaborator Author

Hello @morganney I was thinking that you could use some help, tell me if it's ok ;)

Copy link

codecov bot commented Jun 10, 2024

Codecov Report

Attention: Patch coverage is 97.32771% with 19 lines in your changes missing coverage. Please review.

Project coverage is 97.54%. Comparing base (41f14e2) to head (9aa6299).

Files Patch % Lines
src/baseVisitor.ts 97.32% 14 Missing and 5 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main       #2       +/-   ##
===========================================
+ Coverage   53.06%   97.54%   +44.47%     
===========================================
  Files           2        2               
  Lines         620     1057      +437     
  Branches       12       77       +65     
===========================================
+ Hits          329     1031      +702     
+ Misses        291       21      -270     
- Partials        0        5        +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@maastrich maastrich force-pushed the chore/add-visitors branch 3 times, most recently from e78f82f to dab2fd8 Compare June 10, 2024 23:58
@maastrich
Copy link
Collaborator Author

Almost done, but a few things to take care about aggregate types
I'll take care of them in the morning, it's getting late in paris

@morganney
Copy link
Owner

bonne nuit.

@maastrich
Copy link
Collaborator Author

hello @morganney, I've updated the PR with some tests, do you think this kind of tests works for you ?

@morganney
Copy link
Owner

I think it is a solid approach, I like it. Thanks. I've got a couple questions/comments though if you don't mind.

src/types.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
@morganney
Copy link
Owner

@maastrich I've created branch pr/2-suggested-changes if you wanted to rebase this work off of that. Either that or work from my comments, up to you. I appreciate your help. I'll be able to continue collaborating after work.

@maastrich
Copy link
Collaborator Author

maastrich commented Jun 12, 2024

Hey @morganney, I've made some pretty good advancements, coverage is starting to look good and I've patched some of the flaws

Next step is to reset aggregated types and find a workaround about the name conflicts

I've settled on a few tests skips as I do not know how to trigger these nodes visitors

about the tests utils, I moved them in a utils file as you proposed

PS: I've made another PR about AggregatedTypes -> #5

commit a3c417b
Author: maastrich <pinsaultm@gmail.com>
Date:   Wed Jun 12 20:54:32 2024 +0200

    chore: Remove console.error in AssignmentProperty method

commit 281106d
Author: maastrich <pinsaultm@gmail.com>
Date:   Wed Jun 12 20:46:56 2024 +0200

    chore: Enable decorators in parseSync options

commit b59b57a
Author: maastrich <pinsaultm@gmail.com>
Date:   Wed Jun 12 20:07:49 2024 +0200

    feat: Add missing callbacks for function properties in BaseVisitor

commit 631e4f2
Author: maastrich <pinsaultm@gmail.com>
Date:   Wed Jun 12 18:13:35 2024 +0200

    chore: Update npm dependency to latest stable version

commit 54ee886
Author: maastrich <pinsaultm@gmail.com>
Date:   Wed Jun 12 16:13:18 2024 +0200

    feat: Add missing callbacks for function properties in BaseVisitor

commit e626433
Author: maastrich <pinsaultm@gmail.com>
Date:   Wed Jun 12 16:05:10 2024 +0200

    chore: Update npm dependency to latest stable version

commit fe09515
Author: maastrich <pinsaultm@gmail.com>
Date:   Tue Jun 11 19:27:19 2024 +0200

    feat: Add missing callbacks for function properties in BaseVisitor

commit 5c21cbc
Author: maastrich <pinsaultm@gmail.com>
Date:   Tue Jun 11 17:31:16 2024 +0200

    feat: Add missing callbacks for function properties in BaseVisitor

commit e86fc0c
Author: maastrich <pinsaultm@gmail.com>
Date:   Tue Jun 11 02:28:27 2024 +0200

    feat: Implement all node walkers method

commit dab2fd8
Author: maastrich <pinsaultm@gmail.com>
Date:   Tue Jun 11 01:37:41 2024 +0200

    chore: Implement all node walkers method

commit 8f967ab
Author: Mathis Pinsault <pinsaultm@gmail.com>
Date:   Tue Jun 11 01:56:53 2024 +0200

    chore: add all node walkers method as no implemented (morganney#3)

    * chore: add all node walkers method as no implemented

    * refactor: remove unused import in walk.ts

commit e88099c
Author: Morgan Ney <morganney@gmail.com>
Date:   Mon Jun 10 18:39:17 2024 -0500

    feat: add more nodes. (morganney#4)

    * chore: add style preferences as lint rules.
@maastrich
Copy link
Collaborator Author

@morganney I believe that I'm happy with these changes
I've opened a PR on swc to add a missing type (forcing me to add a @ts-expect-error) i'm hopping that it will be merged en published soon

@morganney
Copy link
Owner

@maastrich mind rebasing with main? I made some changes. Specifically, I wasn't sure why tsconfig.meta.json was necessary so combined it with the main tsconfig file. I also updated the linting and fixed some of those errors.

@maastrich
Copy link
Collaborator Author

@morganney I can revert the meta tsconfig but I usually do that to ensure separation between the actual code and testing/config files
This way, you can ensure that the walker will run on the browser, no node:* needed but they will be defined for the tests 🤔

Your choice

@morganney
Copy link
Owner

@morganney I can revert the meta tsconfig but I usually do that to ensure separation between the actual code and testing/config files This way, you can ensure that the walker will run on the browser, no node:* needed but they will be defined for the tests 🤔

Your choice

Gotcha. Thanks for explaining your reasoning.

I think in this case, for now anyway, can you combine it to just the one tsconfig.json file? If that ends up being a mistake or the other seems necessary, we can add it back in.

@maastrich
Copy link
Collaborator Author

@morganney sure, done

@maastrich maastrich marked this pull request as ready for review June 17, 2024 15:57
@maastrich
Copy link
Collaborator Author

btw I've found few examples in swc main repository for the nodes I couldn't test, few of them are still untested but there aren't that much
about coverage, I believe some types aren't correct from swc as the decorators are never undefined 🤔

The code changes in `baseVisitor.ts` fix an unnecessary condition in the `TsPropertySignature` method. The `for` loop now correctly handles the case when `n.params` is undefined. This change improves the code's readability and ensures proper execution.
Copy link
Owner

@morganney morganney left a comment

Choose a reason for hiding this comment

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

@maastrich great work!

@morganney morganney merged commit c13245d into morganney:main Jun 17, 2024
3 checks passed
@morganney
Copy link
Owner

I can tidy up the README in another PR and then release this later today.

@maastrich maastrich deleted the chore/add-visitors branch June 17, 2024 16:24
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