Skip to content

Allows emacs version to parse json #133

Merged
merged 10 commits into from Aug 14, 2014

3 participants

@siddhanathan

Fixes issue: #132

@timvisher timvisher commented on an outdated diff May 4, 2014
emacs/vimgolf.el
(defun vimgolf-setup (status challenge-id)
+ (if (file-exists-p ".emacs.d/vimgolf.json")
+ (delete-file ".emacs.d/vimgolf.json"))
+ (url-copy-file (vimgolf-challenge-url challenge-id) ".emacs.d/vimgolf.json")
(vimgolf-clear-keystrokes)
@timvisher
Collaborator
timvisher added a note May 4, 2014

I think it makes more sense to put this in a .vimgolf directory. I'm not sure if standard vimgolf has one of these (@igrigorik?). I, for instance, version control my .emacs.d and it would be annoying to have this file start to show up there.

@timvisher
Collaborator
timvisher added a note May 4, 2014

Also, is there any reason not to use the standard url retrieval facilities rather than actually copying the file down?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timvisher timvisher commented on an outdated diff May 4, 2014
emacs/vimgolf.el
(vimgolf-clear-keystrokes)
(setq vimgolf-prior-window-configuration (current-window-configuration)
vimgolf-challenge challenge-id)
(goto-char (point-min))
- (let* ((start-text (vimgolf-read-next-data-chunk))
- (end-text (vimgolf-read-next-data-chunk)))
+ (let* ((start-text (cdr(assq 'data(assq 'in(json-read-file ".emacs.d/vimgolf.json")))))
+ (end-text (cdr(assq 'data(assq 'out(json-read-file ".emacs.d/vimgolf.json"))))))
+
@timvisher
Collaborator
timvisher added a note May 4, 2014

You're not requiring json so json-read-file will fail unless you're lucky enough to be loaded in an emacs that requires it somewhere else.

@timvisher
Collaborator
timvisher added a note May 4, 2014

Weird that there's an extra space here too.

@timvisher
Collaborator
timvisher added a note May 5, 2014

I would format your data read there differently as well. Something like

(challenge-json (json-read-file filename))
(start-text (cdr (assq 'data (assq 'in challenge-json))))
…

Note the spaces.

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

Thanks for doing this! I sadly haven't had enough time to play vimgolf of late… :(

FWIW, if you used url-retrieve… you'd be able to just json read the buffer and avoid all the messiness with where to put the file.

@siddhanathan

I believe that url-retrieve-synchronously does not automatically decompress GZip'd files. The server seems to return a gzip'd file. I got a 'JSON readtable error' while trying to use it.

Writing and reading a local file was just a temporary hack to get this working. I will try url-retrieve some time later.

@timvisher
Collaborator

indeed. hadn't seen that. could be worth a bug report to the emacs core as that functionality clearly should be expected, meaning that if a server sends gzipped data we should get it back unzipped.

@siddhanathan

That last patch fixes the gzip issue. Request.el uses curl instead of url.el to allow automatic decompression of gzip files. The only catch is that it does not come included in emacs. 'request 'request-deferred and 'deferred will need to be dependencies.

@timvisher
Collaborator

@siddhanathan This looks pretty good. I'm not at all familiar with deferred so that's something I might dig into a bit. I'm a little leary of introducing a dependency on curl though as that leaves our Windows friends out in the cold, a bit.

@igrigorik Is there any way to request non-gzipped content from the server?

@timvisher timvisher self-assigned this Aug 8, 2014
@igrigorik
Owner

Skip the Accept-Encoding: gzip HTTP header on the request and it should return the response without applying gzip compression.

@timvisher
Collaborator

@siddhanathan If you can, give what @igrigorik mentioned a try and stick with standard emacs features. If it's so onerous that we can't do it that way then maybe I'll consider using requests.el.

@siddhanathan

I'm not a fan of deferred.el myself. I had to use it to prevent request.el from running in the background.

Short answer: I think it's possible; just need more time to confirm.

Long answer:

Here's the relevant source code from url-vars.el which can do this:

;; FIXME!!  (RFC 2616 gives examples like `compress, gzip'.)
(defvar url-mime-encoding-string nil
  "*String to send in the Accept-encoding: field in HTTP requests.")

From the FIXME tag, it's clear that the bug was known in emacs core for a while. RFC 2616 defines compression in HTTP 1.1

I think, since the encoding wasn't set, the server decides that the client can accept any encoding. This is essentially the same as sending a blank accept-encoding. I'm not sure if there's any way to skip the accept-encoding tag without modifying url.el itself.

We need to disable this behavior, probably by using something like:

(setq url-mime-encoding-string "identity")
(setq url-mime-encoding-string "*;q=0")

Either of the above will end up still sending the accept-encoding tag, but letting the server know that only non-compressed data can be received.

@siddhanathan

Didn't think it'll be that simple. I think it might be worth telling the emacs core developers to change the defaults for url-mime-encoding-string from nil to "identity"

@timvisher Can you merge this?

@timvisher timvisher commented on an outdated diff Aug 9, 2014
emacs/vimgolf.el
@@ -104,6 +106,12 @@ with `C-c C-v` prefixes to help in playing VimGolf.
(defvar vimgolf-end-buffer-name "*vimgolf-end*")
(defvar vimgolf-keystrokes-buffer-name "*vimgolf-keystrokes*")
+; Prevent the server from compressing the response
+(setq url-mime-encoding-string "identity")
+
+; Put url-http-end-of-headers in current scope
+(defvar url-http-end-of-headers)
+
@timvisher
Collaborator
timvisher added a note Aug 9, 2014

this setq and defvar should be done with a let around the specific calls to url.el functions. That way we don't corrupt the value globally. I would probably accept a vimgolf-url-mime-encoding-string var set to "identity".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timvisher timvisher commented on an outdated diff Aug 9, 2014
emacs/vimgolf.el
@@ -292,26 +300,26 @@ unknown key sequence was entered).")
(when (get-buffer buf)
(kill-buffer buf))))
-(defun vimgolf-read-next-data-chunk ()
- "Return the next chunk of data as a string, leaving the point at the end of that chunk."
- (let ((data-start-regexp " data: |\\+\\{0,1\\}\n")
- (data-end-regexp "\\([ ]\\{4\\}\\|[ ]\\{0\\}\\)\n type: [-a-z]+"))
- (unless (re-search-forward data-start-regexp nil t)
- (error "Can't find data in response from vimgolf"))
- (let ((start (point)))
- (unless (re-search-forward data-end-regexp nil t)
- (error "Unclosed data section in response from vimgolf"))
- (let ((str (buffer-substring-no-properties start (match-beginning 0))))
- (replace-regexp-in-string "^ " "" str)))))
+(defun get-text (var response)
@timvisher
Collaborator
timvisher added a note Aug 9, 2014

vimgolf-get-text

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timvisher timvisher commented on an outdated diff Aug 9, 2014
emacs/vimgolf.el
@@ -292,26 +300,26 @@ unknown key sequence was entered).")
(when (get-buffer buf)
(kill-buffer buf))))
-(defun vimgolf-read-next-data-chunk ()
- "Return the next chunk of data as a string, leaving the point at the end of that chunk."
- (let ((data-start-regexp " data: |\\+\\{0,1\\}\n")
- (data-end-regexp "\\([ ]\\{4\\}\\|[ ]\\{0\\}\\)\n type: [-a-z]+"))
- (unless (re-search-forward data-start-regexp nil t)
- (error "Can't find data in response from vimgolf"))
- (let ((start (point)))
- (unless (re-search-forward data-end-regexp nil t)
- (error "Unclosed data section in response from vimgolf"))
- (let ((str (buffer-substring-no-properties start (match-beginning 0))))
- (replace-regexp-in-string "^ " "" str)))))
+(defun get-text (var response)
+ (format "%s" (assoc-default 'data (assq var response))))
+
+(defun read-json (challenge-id)
@timvisher
Collaborator
timvisher added a note Aug 9, 2014

vimgolf-read-json

@timvisher
Collaborator
timvisher added a note Aug 9, 2014

actually make that something like vimgolf-retrieve-challenge

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

@siddhanathan Now we're talking! :+1:

@igrigorik I forget, do we have a preference for squashed or not?

@siddhanathan

@timvisher Nice! So the gzip issue is already fixed in Emacs 24.4

@igrigorik
Owner

+1 for squashed, easier to read diff.

@timvisher timvisher and 1 other commented on an outdated diff Aug 10, 2014
emacs/vimgolf.el
(defun vimgolf-setup (status challenge-id)
+ (let ((defvar url-http-end-of-headers)
+ (url-mime-encoding-string "identitiy"))
@timvisher
Collaborator
timvisher added a note Aug 10, 2014

does (let ((url-mime-encoding-string "identity")) not work? Also, even if this is required, I believe you've got a typo there.

@siddhanathan
siddhanathan added a note Aug 10, 2014

That looked awkward to me on first glance. This is the error I get if I don't use the defvar:

error in process filter: goto-char: Symbol's function definition is void: url-http-end-of-headers

I found another way to bring this variable into scope:

(declare (special url-http-end-of-headers))

I'll end up using this instead, unless there's a better option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timvisher timvisher commented on an outdated diff Aug 10, 2014
emacs/vimgolf.el
(defun vimgolf-setup (status challenge-id)
+ (let ((defvar url-http-end-of-headers)
+ (url-mime-encoding-string "identitiy"))
+ (setq response (vimgolf-retrieve-challenge challenge-id)))
@timvisher
Collaborator
timvisher added a note Aug 10, 2014

vimgolf-response

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timvisher timvisher and 1 other commented on an outdated diff Aug 11, 2014
emacs/vimgolf.el
- "Return the next chunk of data as a string, leaving the point at the end of that chunk."
- (let ((data-start-regexp " data: |\\+\\{0,1\\}\n")
- (data-end-regexp "\\([ ]\\{4\\}\\|[ ]\\{0\\}\\)\n type: [-a-z]+"))
- (unless (re-search-forward data-start-regexp nil t)
- (error "Can't find data in response from vimgolf"))
- (let ((start (point)))
- (unless (re-search-forward data-end-regexp nil t)
- (error "Unclosed data section in response from vimgolf"))
- (let ((str (buffer-substring-no-properties start (match-beginning 0))))
- (replace-regexp-in-string "^ " "" str)))))
+(defun vimgolf-get-text (var response)
+ (format "%s" (assoc-default 'data (assq var response))))
+
+(defun vimgolf-retrieve-challenge (challenge-id)
+ (interactive)
+ (declare (special url-http-end-of-headers))
@timvisher
Collaborator
timvisher added a note Aug 11, 2014

yeah. this is weird. what happens if you just leave it out? i'm wondering because it looks like you're just referencing a free variable down on line 305 which is set in the url-http.el code. You shouldn't need to declare it to use it, we'll just get a warning.

If I have some free time I'll try to dig into this too.

@siddhanathan
siddhanathan added a note Aug 11, 2014

Sorry about that. I just realized the problem was with my config. I tried emacs -Q and it loaded perfectly fine.

@timvisher
Collaborator
timvisher added a note Aug 11, 2014

not exactly sure what you're referencing here. :)

@siddhanathan
siddhanathan added a note Aug 11, 2014

I was referencing the weird error I got when I did not redefine url-http-end-of-headers:
Symbol's function definition is void: url-http-end-of-headers

I can't manage to reproduce it now.

The line (declare (special url-http-end-of-headers)) was unnecessary, as you mentioned earlier.

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

@siddhanathan Great work! Please squash and we'll merge this puppy! :)

@timvisher
Collaborator

@siddhanathan Whoops. Let's actually hold off on the squash, because we need a version bump, a changelog entry, and you added to the list of Contributors. :)

@timvisher
Collaborator

@siddhanathan Also, the CHANGELOG file needs to be created.

@siddhanathan

@timvisher something like this?

@timvisher
Collaborator

Wow! That's way better than what I was hoping for. :)

How about 0.10.0, though, since this is a new feature.

After that, I'll give it one more look over, and then we can squash and push!

@igrigorik igrigorik merged commit 9fd8aaf into igrigorik:master Aug 14, 2014
@igrigorik
Owner

Great stuff, thanks guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.