-
Notifications
You must be signed in to change notification settings - Fork 7
shadow cljs fix #18
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
shadow cljs fix #18
Conversation
da3cdec to
ba6d937
Compare
arichiardi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - haven't tried it though
|
Looks good to me as well - at the very least this is no longer going to blow up with shadow, even if it turns out the completion doesn't work properly. I guess @thheller can potentially confirm if we're using the right shadow-cljs API right now. Other than this - seems you've reflowed the entire README (I see most like breaks are gone), so it's hard to understand what are the documentation changes, but I'll assume those are fine. I was thinking it will be nice if there was a section in the docs about hacking on suitable - should be useful to potential contributors. |
|
One more thing - if this API didn't exist before shadow 2.10, we should probably guard against this and just return an empty list of candidates. |
|
LGTM. I'll make sure that |
|
@bbatsov the pure diff of README.md, only minor changes. Sorry the line breaks did bother me. |
c554891 to
8472261
Compare
0c96b04 to
d52351c
Compare
|
I saw you already cut 0.4, so I guess it's time to merge this branch. Big thanks for working on this! |

I believe that this fixes #15. I'll test this some and would invite others testing as well.