-
Notifications
You must be signed in to change notification settings - Fork 21
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
General changes #44
General changes #44
Conversation
- Calm Emacs Lisp package lint - Deprecate monroe-old-style-stacktraces for monroe-print-stack-trace-function
- Change bdecode functions, use skeeto bencode (with a small change to allow string keywords) - Change the way monroe encode/decode using assoc for dictionaries and use plist for the return list of values - Send the namespace only when is not nil
Sometimes people ask me "why use monroe when cider has more features?" and I'm like "monroe is much simpler and easier to understand; for instance, it's just one file instead of a ton of different files" Moving from "it's all in one file" to "it's multiple files" is kind of a big change, and I don't think it should be done without some discussion. Maybe it's worth it, but personally I'd prefer if it was all in one. Also, the new version is dramatically longer; what are the shortcomings of the old implementation which motivate the move to a more complex implementation? Not to say we shouldn't do it, but we should be clear about the trade-offs. |
Yeah, I'm with @technomancy here. At first, I was confused with the amount of changes. Why new bencode, what were the problems with the previous one? Some monroe general guidelines & philosophy:
|
Thanks for the feedback
This new and longer implementation of bencode it's very well documented, It as specific error types for parsing errors and in my opinion even tho it's longer, it easier to debug if something goes wrong.
I understand, my idea was to fully support the NREPL protocol but not Clojure specific (which as @technomancy point out I would have to ping the NREPL team to maybe change some things). I wasn't aware of these constrains which I think are complete reasonable, but not in line for what I had in mid to contribute for this project, sorry for the misunderstanding. As my intentions are not align with the project, I'll continue with the fork (maybe change the name given that I'll probably diverge even more). Again, thanks for writing and maintaining monroe, I'll be closing now this merge request and the issue associated with it. Good luck! 👋 |
Thank you @Sasanidas ! If you happen to make any improvements that could be merged in monroe, I'll be more than happy to check them :) |
As mention in #43, I've been playing around with monroe and modifying it so it ca be used for more servers other than Clojure.
These are my firsts steps, which are not yet addressing the #43 issue, but I think it can help to improve the package.
The changes so far are:
I've been testing these new changes with two nprel servers:
Both seems to work "good enough", but I'm sure there are still bugs to fix.