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

Use built-in query building function #20

Closed
wants to merge 1 commit into from

Conversation

basil-conto
Copy link
Contributor

The function url-build-query-string generalises ghub--url-encode-params, but expects a list of true lists, rather than cons pairs.

ghub.el Outdated
(url-build-query-string
(mapcar (lambda (cell)
(list (car cell) (cdr cell)))
params)))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternatively:

(require 'map)

;;; ...

(defun ghub--url-encode-params (params)
  (url-build-query-string (map-apply #'list params)))

@tarsius
Copy link
Member

tarsius commented Jun 13, 2017

If we could just replace ghub--url-encode-params with url-build-query-string, then I would be in favor of that. But we can't because the latter expects a format that doesn't match our public interface.

I might also be okay with replacing ghub--url-encode-params with (url-build-query-string (map-apply #'list params)), directly in ghub--request. But we can't because map isn't available in all Emacsen that we support.

Since we cannot actually remove ghub--url-encode-params, I want to keep the explicit and easy to understand implementation instead of redirecting to the much more complicated url-build-query-string.

@tarsius tarsius closed this Jun 13, 2017
@basil-conto
Copy link
Contributor Author

Thanks, I appreciate and understand your reservations.

I just have one question - AFAICT, map.el was included in Emacs precisely between versions 24.5-rc2 and 25.1.91, and ghub.el depends on Emacs 25. Does this mean that there are versions of Emacs v such that 25 <= v < 25.1.91 which ghub.el officially supports but which lack map.el? Or am I missing / have I misunderstood something?

@basil-conto
Copy link
Contributor Author

I just remembered that this PR also includes the change s/cl-delete/assq-delete-all/. I think this should be applied regardless of the URL parameter encoding, as cl-lib/cl-seq is not explicitly loaded. The alternative would be to load one of these, but I would argue that is unnecessary in the face of an equivalent built-in function. Should I break this change out into a separate PR? (I seem to have a problem with almost-atomic-PRs.)

@tarsius
Copy link
Member

tarsius commented Jun 14, 2017

I've added the dependency on cl-lib. assq-delete-all is a bit to peculiar for my taste, it handles a very narrow use-case and it also is the only assq-* function.

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.

None yet

2 participants