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

Enable to set coding system for encoding/decoding #85

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@shigemk2

shigemk2 commented Jan 6, 2018

Non-ascii strings in JSON data would be unicoded when POSTing curl with JSON data because
buffer-file-coding-system in data buffer is 'binary.

But to change this code is not good for web page whose encoding is other encoding, so
set encode in variable request-conding-system.

Example:

(setq request-conding-system 'utf-8)
(request
 "http://httpbin.org/put"
 :type "PUT"
 :data (json-encode '(("key" . "値1") ("key2" . "値2")))
 :headers '(("Content-Type" . "application/json"))
 :parser 'json-read
 :success (cl-function
           (lambda (&key data &allow-other-keys)
             (message "I sent: %S" (assoc-default 'json data)))))
Enable to set coding system for encoding/decoding
Non-ascii strings in JSON data would be unicoded when POSTing curl with JSON data because
buffer-file-coding-system in data buffer is 'binary.

But to change this code is not good for web page whose encoding is other encoding, so
set encode in variable request-conding-system.

(seq request-conding-system 'utf-8)
@shigemk2

This comment has been minimized.

shigemk2 commented Jan 6, 2018

Duplicate: #79

But we should not use hard-coding, so I created variables.

@alphapapa

This comment has been minimized.

alphapapa commented Jul 9, 2018

@shigemk2

  1. I think you meant request-coding-system, not request-conding-system.
  2. Surely the best default is 'utf-8 not binary, right? I doubt we should ever use binary encoding unless uploading a binary file.
  3. Instead of using a global, special variable, why not add it as a keyword argument to request, like everything else?
@shigemk2

This comment has been minimized.

shigemk2 commented Oct 23, 2018

@alphapapa

I think you meant request-coding-system, not request-conding-system.

It's a just typo. Sorry.

Surely the best default is 'utf-8 not binary, right? I doubt we should ever use binary encoding unless uploading a binary file.

I am not sure why emacs-request uses binary encoding. I want to use utf-8 as default.

Instead of using a global, special variable, why not add it as a keyword argument to request, like everything else?

We should use emacs-request like curl
, but specifying encoding in keyword argument is not like using curl.

@alphapapa

This comment has been minimized.

alphapapa commented Oct 27, 2018

@shigemk2

We should use emacs-request like curl
, but specifying encoding in keyword argument is not like using curl.

I don’t understand what you mean.

Also, can you explain more why this PR is needed?

@shigemk2

This comment has been minimized.

shigemk2 commented Oct 29, 2018

@alphapapa

Instead of using a global, special variable, why not add it as a keyword argument to request, like everything else?

Sorry, I didn't understand what you mean.

(request
 "http://httpbin.org/post"
 :type "POST"
 :files `(("current buffer" . ,(current-buffer))
          ("data" . ("data.csv" :data "1,2,3\n4,5,6\n")))
 :parser 'json-read
 :success (cl-function
           (lambda (&key data &allow-other-keys)
             (message "I sent: %S" (assoc-default 'files data)))))

Do you mean adding keyword argument like this?

(request
 "http://httpbin.org/post"
 :type "POST"
 :encoding 'utf-8
 :files `(("current buffer" . ,(current-buffer))
          ("data" . ("data.csv" :data "1,2,3\n4,5,6\n")))
 :parser 'json-read
 :success (cl-function
           (lambda (&key data &allow-other-keys)
             (message "I sent: %S" (assoc-default 'files data)))))
@titaniumbones

This comment has been minimized.

Collaborator

titaniumbones commented Nov 29, 2018

A simpler option would be to just change 'binary to 'utf-8, as @DamienCassou did in DamienCassou#3 . What do you think?

@alphapapa

This comment has been minimized.

alphapapa commented Nov 29, 2018

Do you mean adding keyword argument like this?

Yes, I think that would be good.

@titaniumbones That is probably also a good idea, although it may merit some testing before pushing to master. It wouldn't surprise me if that had some edge cases in packages that use request.

README.rst Outdated
.. code:: emacs-lisp
(setq request-conding-system 'utf-8)

This comment has been minimized.

@ardumont

ardumont Nov 29, 2018

I see it's consistent everywhere.
Still shouldn't it be coding instead of conding?

Suggested change Beta
(setq request-conding-system 'utf-8)
(setq request-coding-system 'utf-8)

This comment has been minimized.

@shigemk2
@titaniumbones

This comment has been minimized.

Collaborator

titaniumbones commented Nov 29, 2018

I have rebased this branch off of current master and added two commits here: https://github.com/tkf/emacs-request/tree/data-coding-system .

The new commits:

  • first rename request-conding-system to reqeust-coding-system, but then
  • replace request-coding-system with a new key :coding-system to request and its subsidiary functions

I haven't done my own tests but when tested on travis they seem to work except in Emacs 25.1. I'd be grateful if anyone else could take a look at the build failure here: https://travis-ci.org/tkf/emacs-request/builds/461192047 and see if you can understand why it would fail in that particular emacs version. (looks like the one failure, which timed out, was an artifact of the build process. All tests pass now.)

@shigemk2 I have submitted a PR from my new branch to your branch 😄 . that is a little convoluted but I thought it was one way for you to test my code, perhaps.

@alphapapa

This comment has been minimized.

alphapapa commented Nov 30, 2018

@titaniumbones Please consider :encoding instead of :coding-system. The brevity would be appreciated, and it's just as clear. The docstring can explain the parallel with encode-coding-string.

titaniumbones added a commit that referenced this pull request Nov 30, 2018

@titaniumbones

This comment has been minimized.

Collaborator

titaniumbones commented Nov 30, 2018

@titaniumbones Please consider :encoding instead of :coding-system. The brevity would be appreciated, and it's just as clear. The docstring can explain the parallel with encode-coding-string.

@alphapapa done in fa58c18 , except for the docstring; I'd welcome a fix there as I am not quite sure what to say and am very sleepy 😄 .

As for merging: what would you suggest we do before committing o master? This is the first substantive bug fix in quite a while, it would be nice to push it out so package maintainers can stop monkeypatching/making ad-hoc fixes. But OTOH i don't want to break people's workarounds.

@alphapapa

This comment has been minimized.

alphapapa commented Dec 1, 2018

For the docstring, I would suggest changing the line in question to:

Coding system for submitted data (default: `utf-8').  See Info node `(elisp)Coding Systems`.

As for merging: what would you suggest we do before committing o master? This is the first substantive bug fix in quite a while, it would be nice to push it out so package maintainers can stop monkeypatching/making ad-hoc fixes. But OTOH i don't want to break people's workarounds.

Well, it's a bit of a pain, but it seemed to work well for us when making changes to Outshine: I suggest posting a message to /r/emacs (and maybe emacs-devel), explaining what's going on, and asking package developers to test their packages with whatever branch you push this to. Then wait 4-6 weeks, and if no one reports a problem, merge to master. I realize that 4-6 weeks seems like a long time, but package authors work at their own pace.

titaniumbones added a commit that referenced this pull request Dec 2, 2018

@titaniumbones

This comment has been minimized.

Collaborator

titaniumbones commented Dec 2, 2018

OK, I've created a new branch "development" https://github.com/tkf/emacs-request/commits/development that includes these changes. And we can target that branch for future merges as well. The goal will maybe be to post that info to a broader audience at hte end of next week.

@shigemk2

This comment has been minimized.

shigemk2 commented Dec 3, 2018

@titaniumbones
#85 (comment)
Thank you so much!!

@titaniumbones

This comment has been minimized.

Collaborator

titaniumbones commented Dec 6, 2018

OK, these change shave been merged into the development branch (and actually superseded in part by 081f6bf and 9e3ef2d). So I'll close this PR and perhaps merge after following the strategy suggested by @alphapapa in #85 (comment)

1 similar comment
@titaniumbones

This comment has been minimized.

Collaborator

titaniumbones commented Dec 6, 2018

OK, these change shave been merged into the development branch (and actually superseded in part by 081f6bf and 9e3ef2d). So I'll close this PR and perhaps merge after following the strategy suggested by @alphapapa in #85 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment