-
Notifications
You must be signed in to change notification settings - Fork 7
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
Improvements rackjure would need #1
Comments
Thanks a lot for opening this! I think it's really helpful to see how rackjure has evolved. Could I ask you to split these into separate issues? That way it'll be easier to track and discuss them individually. |
Certainly, I'd be happy to do that. Before I do: Another reason I presented them together is that, if you liked all of them, then one possible implementation approach would be to start over with rackjure's That might be easier than doing it the other way around. Of course, I realize that approach may only seem easier to me because I'm more familiar with that other code. :) But seriously, it might actually be smoother. At least, I wanted to suggest the possibility. However if after considering that, you'd still prefer to do it incrementally, I'd understand. |
The main reason I want to split these issues is to make it easier for me to understand why each of these decisions are necessary. I think that it's quite likely that the motivations translate to Racket, but I'm also a little bit wary of accommodating Clojure problems if I'm not sure they also apply to Racket itself. |
Done! |
...and now all the individual issues are done as well! Thanks a lot for the help—the lexical context one was probably an especially good change. Definitely let me know if you come up with any other issues. For now, I'll consider this resolved. |
Thanks!! This morning (my time) I left a couple comments on #3 and #5, I don't think they're 100% closed yet. When everything does settle down, I'd ask you to bump the So close!! |
First, thanks again for taking the initiative to have one, go-to threading package.
I mentioned a few ways in which rackjure's threading macros had evolved, that you might want to consider, and that rackjure would need in order to drop its own module and use your package instead.
I thought it would be easiest to present/consider them all together, at least initially. However if you'd prefer me to open each as its own issue, I'd be happy to do that.
quote
formsRackjure's threading macros have special cases so that
(~> v 's)
-- which is really(~> v (quote s))
-- expands to((quote s) v)
not(quote v s)
. The latter obviously isn't useful, but why is the former? Because rackjure's#%app
supports applying a symbol to dictionary, to be adict-ref
.This simply requires an extra syntax parse clause such as:
However, the template will become a little more complicated than that after addressing the remaining issues.
~>
syntaxDoing so ensures that Racket's macro expander will use the
#%app
in that lexical context.This is discussed in rackjure issue 25. The issue references two commits, because it took me two attempts to get it right :) and I was less compulsive back then about squashing commits before pushing :).
This is discussed in rackjure issue 28.
The commit fixing this also involved some pretty substantial refactoring of the code by @qerub. Because of that, and because CLJ-1121 is arguably a corner case, I'm not sure how enthusiastic you'll be about this, but I wanted to point it out.
You might be least enthusiastic about this. On the one hand, it's probably easier than you think, and I'd rather not drop an old Racket version for rackjure unless necessary. On the other hand, I only knew one person using it on a version that old, and that was probably 2 years ago, so maybe I could change this.
When you have time, please let me know what you think about these? Also, do you have any preferences about trying to incorporate these changes yourself, vs. me giving you pull request(s)?
Thanks again!
The text was updated successfully, but these errors were encountered: