-
-
Notifications
You must be signed in to change notification settings - Fork 3
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
update to use JuliaSyntax.jl v1.0.0 #9
Conversation
051b17d
to
cdd5803
Compare
Brilliant, thanks for doing this work Chengyu! |
1f35511
to
2680c8b
Compare
I think this pr is ready for review. But not sure when to merge it. One idea is to keep this pr open, and
Otherwise this pr will not pass CI. (Update) |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9 +/- ##
==========================================
+ Coverage 66.66% 66.87% +0.21%
==========================================
Files 1 1
Lines 156 157 +1
==========================================
+ Hits 104 105 +1
Misses 52 52 ☔ View full report in Codecov by Sentry. |
Great! I'll try to review this PR in the week, seems like it should be good to just merge? Feel free to ping me if I don't get around to this by the weekend 🙂 |
The source changes: 1. Replace K"true" and K"false" with K"Bool" JuliaLang/JuliaSyntax.jl@b92fc5e5dab7 2. Rename haschildren() to is_leaf() JuliaLang/JuliaSyntax.jl@d8796c6ac136 Also update the tests and doctests to reflect a change in the syntax pass.
Instead of just reaching straight into the private fields of the relevant structs, (import and) use the methods provided by JuliaSyntax. Specifically: 1. length(node.args) -> numchildren(node) 2. isempty(children(node)) -> numchildren(node) == 0 3. node.args -> children(node) 4. node.args[x] -> node[x] 5. node.span -> span(node)
Sorry it's dragged on, I've just looked over these changes locally and they look good to me 👍. Thanks again for your work on this. I'd like to fast-forward merge this, but rebasing the work into these three commits:
Let me know if you're happy with this 🙂 |
I'm fine with those changes. Feel free to |
I've just checked and |
2680c8b
to
c221b4e
Compare
Thanks for the help! |
This pr should be merged with the PR that updates the JuliaSyntax.jl version.
The tests here are currently invalid (because JuliaSyntax.jl has not been upgraded yet)
JuliaSyntax.jl Breaking changes: