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

General changes #44

Closed
wants to merge 6 commits into from
Closed

General changes #44

wants to merge 6 commits into from

Conversation

Sasanidas
Copy link
Contributor

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:

  • Fix some comments (what I mean by fix is to calm down the emacs checkdock and package-lint)
  • Change the bencode, I thought of copying the clojure-nrepl bencode functions, but then I found https://github.com/skeeto/emacs-bencode and with some tweaks I addapt it for the monroe requirements.
  • Small changes here and there (mostly related to the new bencoder)

I've been testing these new changes with two nprel servers:

  • python-nrepl (my own nrepl with python)
  • lein (whith the default command "lein trampoline repl :headless")

Both seems to work "good enough", but I'm sure there are still bugs to fix.

- 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
@technomancy
Copy link
Contributor

technomancy commented Dec 15, 2022

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.

@sanel
Copy link
Owner

sanel commented Dec 15, 2022

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:

  1. Should be simple. KISS.
  2. Single file. No dependency. Support for older Emacs-es.
  3. Must work out of the box. Download a single file and run it.
  4. Keep the previous behavior.
  5. Stable. No "seems to work" :) Either cover all the cases or none (probably the main pain point is not having extensive tests for it, but TBH I'm not sure how to properly test it either).

@Sasanidas
Copy link
Contributor Author

Thanks for the feedback

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.

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.

Some monroe general guidelines & philosophy:
Should be simple. KISS.
Single file. No dependency. Support for older Emacs-es.
Must work out of the box. Download a single file and run it.
Keep the previous behavior.
Stable. No "seems to work" :) Either cover all the cases or none (probably the main pain point is not having extensive tests for it, but TBH I'm not sure how to properly test it either).

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! 👋

@Sasanidas Sasanidas closed this Dec 16, 2022
@sanel
Copy link
Owner

sanel commented Dec 16, 2022

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 :)

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.

3 participants