Skip to content

[Fix: #3786] Sort dictionaries by key in nrepl-bencode #3814

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

iarenaza
Copy link
Contributor

CIDER doesn't adhere to Bencode spec which requires dictionary keys to be sorted alphabetically. This hasn't been a problem so far because the bencode reader on nREPL side doesn't validate the order of keys. Still, it will be rigorous to produce correct values according to the selected format.

[Re: #3786]

Replace this placeholder text with a summary of the changes in your PR.
The more detailed you are, the better.


Before submitting the PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

  • The commits are consistent with our contribution guidelines
  • You've added tests (if possible) to cover your change(s)
  • All tests are passing (eldev test)
  • All code passes the linter (eldev lint) which is based on elisp-lint and includes
  • You've updated the changelog (if adding/changing user-visible functionality)
  • You've updated the user manual (if adding/changing user-visible functionality)

Thanks!

If you're just starting out to hack on CIDER you might find this section of its
manual
extremely useful.

nrepl-client.el Outdated
@@ -423,13 +423,25 @@ decoded message or nil if the strings were completely decoded."
(erase-buffer)
(cons string-q response-q))))

(defun nrepl--bencode-dict (dict)
"Encode DICT with bencode."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably we should mention in the docstring that the keys need to be sorted as per the spec, as this is probably not obvious to people not familiar with bencode - until recently even I didn't know this was in the spec. :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

CIDER doesn't adhere to Bencode spec which requires dictionary keys to
be sorted alphabetically. This hasn't been a problem so far because
the bencode reader on nREPL side doesn't validate the order of
keys. Still, it will be rigorous to produce correct values according
to the selected format.
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.

2 participants