Getting rid of low-level assumptions in yasnippet

Getting rid of low-level assumptions in yasnippet

[Stefan Monnier]

Hi João,

Could you double check the patch below, to make sure it does the right thing?
The main thrust is to get rid of the use of the `cl-struct-slots'
property which doesn't exist in Emacs-25 any more.

This is against the elpa.git version of the code, which IIUC is still
not up-to-date, right?


        Stefan


diff --git a/packages/yasnippet/yasnippet.el b/packages/yasnippet/yasnippet.el
index aa3e312..7b4c429 100644
--- a/packages/yasnippet/yasnippet.el
+++ b/packages/yasnippet/yasnippet.el
@@ -1,6 +1,6 @@
 ;;; yasnippet.el --- Yet another snippet extension for Emacs.
 
-;; Copyright (C) 2008-2013 Free Software Foundation, Inc.
+;; Copyright (C) 2008-2013, 2015 Free Software Foundation, Inc.
 ;; Authors: pluskid <pluskid@gmail.com>,  João Távora <joaotavora@gmail.com>
 ;; Maintainer: João Távora <joaotavora@gmail.com>
 ;; Version: 0.8.0
@@ -856,7 +856,7 @@ Honour `yas-dont-activate', which see."
 \f
 ;;; Internal structs for template management
 
-(defstruct (yas--template (:constructor yas--make-blank-template))
+(defstruct (yas--template (:constructor yas--make-template))
   "A template for a snippet."
   key
   content
@@ -872,16 +872,6 @@ Honour `yas-dont-activate', which see."
   table
   )
 
-(defun yas--populate-template (template &rest args)
-  "Helper function to populate TEMPLATE with properties."
-  (while args
-    (aset template
-          (position (intern (substring (symbol-name (car args)) 1))
-                    (mapcar #'car (get 'yas--template 'cl-struct-slots)))
-          (second args))
-    (setq args (cddr args)))
-  template)
-
 (defstruct (yas--table (:constructor yas--make-snippet-table (name)))
   "A table to store snippets for a particular mode.
 
@@ -1668,20 +1658,19 @@ Optional PROMPT sets the prompt to use."
          (uuid (or (ninth snippet)
                   name))
          (template (or (gethash uuid (yas--table-uuidhash snippet-table))
-                       (yas--make-blank-template))))
+                       (yas--make-template :uuid uuid
+                                           :table snippet-table))))
     ;; X) populate the template object
     ;;
-    (yas--populate-template template
-                           :table       snippet-table
-                           :key         key
-                           :content     (second snippet)
-                           :name        (or name key)
-                           :group       group
-                           :condition   condition
-                           :expand-env  (sixth snippet)
-                           :file        (seventh snippet)
-                           :keybinding  keybinding
-                           :uuid         uuid)
+    (setf (yas--template-key template)        key)
+    (setf (yas--template-content template)    (second snippet))
+    (setf (yas--template-name template)       (or name key))
+    (setf (yas--template-group template)      group)
+    (setf (yas--template-condition template)  condition)
+    (setf (yas--template-expand-env template) (sixth snippet))
+    (setf (yas--template-file template)       (seventh snippet))
+    (setf (yas--template-keybinding template) keybinding)
+
     ;; X) Update this template in the appropriate table. This step
     ;;    also will take care of adding the key indicators in the
     ;;    templates menu entry, if any
@@ -2113,11 +2102,10 @@ This function does nothing if `yas-use-menu' is nil."
                           hash)
       (dolist (uuid omit-items)
         (let ((template (or (gethash uuid hash)
-                            (yas--populate-template (puthash uuid
-                                                             (yas--make-blank-template)
-                                                             hash)
-                                                    :table table
-                                                    :uuid uuid))))
+                            (puthash uuid
+                                     (yas--make-template :table table
+                                                         :uuid uuid)
+                                     hash))))
           (setf (yas--template-menu-binding-pair template) (cons nil :none)))))))
 
 (defun yas--define-menu-1 (table menu-keymap menu uuidhash &optional group-list)
@@ -2125,12 +2113,12 @@ This function does nothing if `yas-use-menu' is nil."
   (dolist (e (reverse menu))
     (cond ((eq (first e) 'yas-item)
            (let ((template (or (gethash (second e) uuidhash)
-                               (yas--populate-template (puthash (second e)
-                                                               (yas--make-blank-template)
-                                                               uuidhash)
-                                                      :table table
-                                                      :perm-group group-list
-                                                      :uuid (second e)))))
+                               (puthash (second e)
+                                        (yas--make-template
+                                         :table table
+                                         :perm-group group-list
+                                         :uuid (second e))
+                                        uuidhash))))
              (define-key menu-keymap (vector (gensym))
                (car (yas--template-menu-binding-pair-get-create template :stay)))))
           ((eq (first e) 'yas-submenu)
@@ -2622,12 +2610,11 @@ whether (and where) to save the snippet, then quit the window."
          (yas--current-template
           (and parsed
                (fboundp test-mode)
-               (yas--populate-template (yas--make-blank-template)
-                                      :table       nil ;; no tables for ephemeral snippets
-                                      :key         (first parsed)
-                                      :content     (second parsed)
-                                      :name        (third parsed)
-                                      :expand-env  (sixth parsed)))))
+               (yas--make-template :table       nil ;; no tables for ephemeral snippets
+                                   :key         (first parsed)
+                                   :content     (second parsed)
+                                   :name        (third parsed)
+                                   :expand-env  (sixth parsed)))))
     (cond (yas--current-template
            (let ((buffer-name (format "*testing snippet: %s*" (yas--template-name yas--current-template))))
              (kill-buffer (get-buffer-create buffer-name))

Re: Getting rid of low-level assumptions in yasnippet

[João Távora]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> Could you double check the patch below, to make sure it does the right thing?
> The main thrust is to get rid of the use of the `cl-struct-slots'
> property which doesn't exist in Emacs-25 any more.

The patch look good. I've pushed it upstream and it passes all
tests. I'm slightly embarassed you have to wade through that junk, but
that's how much Lisp I knew at the time.

> This is against the elpa.git version of the code, which IIUC is still
> not up-to-date, right?

Again embarassing: there are two or three issues on the upstream side
also pending on this move. I'll try to do this later today.

One thing I don't know how to handle is the Git submodules which are
problematic for some (unofficial) uses of ELPA.git, such as
https://github.com/quelpa/quelpa (CCing akira)

So unless you disagree, I'll remove them completely in the subtree merge
result and (temporarily) replace yasnippet/snippets with a copy of the
old snippet collection.

João

Re: Getting rid of low-level assumptions in yasnippet

[Stefan Monnier]

>> Could you double check the patch below, to make sure it does the right thing?
>> The main thrust is to get rid of the use of the `cl-struct-slots'
>> property which doesn't exist in Emacs-25 any more.
> The patch look good.  I've pushed it upstream and it passes all tests.

Great, thanks.

> One thing I don't know how to handle is the Git submodules which are
> problematic for some (unofficial) uses of ELPA.git, such as
> https://github.com/quelpa/quelpa (CCing akira)

Indeed git submodules don't work in elpa.git.  The cleanest way would be
to divide yasnippet into separate (sub)packages.

> So unless you disagree, I'll remove them completely in the subtree merge
> result and (temporarily) replace yasnippet/snippets with a copy of the
> old snippet collection.

Sure,


        Stefan

Re: Getting rid of low-level assumptions in yasnippet

[João Távora]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> So unless you disagree, I'll remove them completely in the subtree merge
>> result and (temporarily) replace yasnippet/snippets with a copy of the
>> old snippet collection.
> Sure,

I finally did the sync, and some "giv mv"s after it looks OK. Here's the
summary:

    commit 39a27b6cd6ced60e81145efb18d72d6081eac9f7
    Merge: 73701d9 a467019
    Author: João Távora <joaotavora@gmail.com>
    Date:   Sat Mar 28 15:16:36 2015 +0000
     
        Update packages/yasnippet by merging from its external upstream subtree
        
        The snippet collection and the "yasmate" tool, realized in the
        upstream as git submodules, are removed in the merge result. The "old"
        snippet collection is however still in place in
        packages/yasnippet/snippets.
        
        Conflicts:
        	.gitignore
                <a lot of conflicts>


I couldn't be extra sure since the instructions in the README failed for
me. Both the "install in place" and the "locally deploy an archive"
failed with some errors. I didn't investigate much, compilation seems to
stop at ergoemacs.

    In toplevel form:
    packages/ergoemacs-mode/ergoemacs-macros.el:265:1:Error: Wrong type argument: listp, value
    make[1]: *** [packages/ergoemacs-mode/ergoemacs-macros.elc] Error 1
    make: *** [all-in-place] Error 2

I did load the package manually and it worked well though.

João

Re: Getting rid of low-level assumptions in yasnippet

[Stefan Monnier]

> I finally did the sync,

Thanks.

> I couldn't be extra sure since the instructions in the README failed for
> me. Both the "install in place" and the "locally deploy an archive"
> failed with some errors. I didn't investigate much, compilation seems to
> stop at ergoemacs.

Hmm... I just tried a "make" and bumped into a few issues (which I just
fixed), but none of them in ergoemacs.  If you can reproduce them,
please report them as bugs.

While trying this "make" I saw that yasnippet compiles cleanly (except
for some issues in the tests which I just fixed), so it looks good,


        Stefan

Re: Getting rid of low-level assumptions in yasnippet

[João Távora]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> I finally did the sync,
>
> Thanks.
>
>> I couldn't be extra sure since the instructions in the README failed for
>> me. Both the "install in place" and the "locally deploy an archive"
>> failed with some errors. I didn't investigate much, compilation seems to
>> stop at ergoemacs.
>
> Hmm... I just tried a "make" and bumped into a few issues (which I just
> fixed), but none of them in ergoemacs.  If you can reproduce them,
> please report them as bugs.
>
Thanks for fixing those issues (I also bumped into them at some point). 

Ergoemacs broke because it is an external and I was using an outdated
buggy version of it. A (require 'cl) issue, that you since fixed...

I should have read the README better, "make externals" **always** needs
to come before "make" even if you don't want to 'make' the external
packages.

João

Re: Getting rid of low-level assumptions in yasnippet

[Matthew Fidler]

[-- Attachment #1: Type: text/plain, Size: 1236 bytes --]

I'm not sure exactly what this is about, but thank you.

Since I'm here I would also like to add that eieio doesn't work the same in
emacs 25.

https://github.com/ergoemacs/ergoemacs-mode/issues/330
On Mar 28, 2015 4:23 PM, "João Távora" <joaotavora@gmail.com> wrote:

> Stefan Monnier <monnier@IRO.UMontreal.CA> writes:
>
> >> I finally did the sync,
> >
> > Thanks.
> >
> >> I couldn't be extra sure since the instructions in the README failed for
> >> me. Both the "install in place" and the "locally deploy an archive"
> >> failed with some errors. I didn't investigate much, compilation seems to
> >> stop at ergoemacs.
> >
> > Hmm... I just tried a "make" and bumped into a few issues (which I just
> > fixed), but none of them in ergoemacs.  If you can reproduce them,
> > please report them as bugs.
> >
> Thanks for fixing those issues (I also bumped into them at some point).
>
> Ergoemacs broke because it is an external and I was using an outdated
> buggy version of it. A (require 'cl) issue, that you since fixed...
>
> I should have read the README better, "make externals" **always** needs
> to come before "make" even if you don't want to 'make' the external
> packages.
>
> João
>

[-- Attachment #2: Type: text/html, Size: 1822 bytes --]

Re: Getting rid of low-level assumptions in yasnippet

[Stefan Monnier]

> Since I'm here I would also like to add that eieio doesn't work the same in
> emacs 25.

There's been many changes to EIEIO's internals, indeed.  I've tried to
preserve compatibility so far both for source files and for
byte-compiled files (tho not for files byte-compiled with non-released
Emacs versions).
So if you experience incompatibilities because of the new code, please
report them,


        Stefan

Re: Getting rid of low-level assumptions in yasnippet

[Matthew Fidler]

[-- Attachment #1: Type: text/plain, Size: 847 bytes --]

Ok,

I believe the problem is with named components.  In the old eieio, it
allowed nil names, but in the current implementation it requires a name for
named components.

A work-around in ergoemacs-mode shows the possible issue

https://github.com/ergoemacs/ergoemacs-mode/commit/71319c2cc1874248c885cd6b6e7f826fb24c49a7

Matt.

On Sun, Mar 29, 2015 at 7:47 PM, Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> > Since I'm here I would also like to add that eieio doesn't work the same
> in
> > emacs 25.
>
> There's been many changes to EIEIO's internals, indeed.  I've tried to
> preserve compatibility so far both for source files and for
> byte-compiled files (tho not for files byte-compiled with non-released
> Emacs versions).
> So if you experience incompatibilities because of the new code, please
> report them,
>
>
>         Stefan
>

[-- Attachment #2: Type: text/html, Size: 1441 bytes --]

Re: Getting rid of low-level assumptions in yasnippet

[Stefan Monnier]

> I believe the problem is with named components.  In the old eieio, it
> allowed nil names, but in the current implementation it requires a name for
> named components.

Actually, (clone <foo> nil) in Emacs-24.4 did not pass nil as the object
name either and signaled the same error, so the problem is elsewhere.
IOW, in Emacs-24.4, this same "clone" call did not receive nil as
argument, but received a string.  So the problem seems to be that
(oref global object-name) returns nil now.

I think the problem is in the fact that ergoemacs uses a mix of "native
EIEIO object names" (the thing which is now deprecated) and "proper
eieio-named names".  E.g. it does things like

          (clone ergoemacs-theme-component-maps--curr-component
                 (concat (oref ergoemacs-theme-component-maps--curr-component object-name) "::" version)))

which passes a string as "native EIEIO object name" and it also uses things then later on in
ergoemacs-copy-obj it expects (oref global object-name) to return
that name.  Instead it should use :object-name

          (clone ergoemacs-theme-component-maps--curr-component
                 :object-name (concat (oref ergoemacs-theme-component-maps--curr-component object-name) "::" version)))

With the patch below I get further but bump into another bug where
ergoemacs seems to use the symbol `standard' as the name of an object
(if I understood the backtrace correctly).  I have to leave now, so
here's what I have so far, which seems to be working (the "format"
thingy is just the current workaround.  Not sure if the bug is on Emacs
side or not).


        Stefan


diff --git a/lisp/emacs-lisp/eieio-base.el b/lisp/emacs-lisp/eieio-base.el
index 5b3d902..86235bd 100644
--- a/lisp/emacs-lisp/eieio-base.el
+++ b/lisp/emacs-lisp/eieio-base.el
@@ -498,6 +498,14 @@ All slots are unbound, except those initialized with PARAMS."
                         (concat nm "-1")))))
     nobj))
 
+(cl-defmethod make-instance ((class (subclass eieio-named)) &rest args)
+  (if (not (stringp (car args)))
+      (cl-call-next-method)
+    (funcall (if eieio-backward-compatibility #'ignore #'message)
+             "Obsolete: object-name passed without :object-name to %S constructor"
+             (car args) class)
+    (apply #'cl-call-next-method class :object-name args)))
+
 (provide 'eieio-base)
 
 ;;; eieio-base.el ends here




diff --git a/ergoemacs-theme-engine.el b/ergoemacs-theme-engine.el
index 6c0b922..4cca730 100644
--- a/ergoemacs-theme-engine.el
+++ b/ergoemacs-theme-engine.el
@@ -2162,7 +2163,7 @@ Allows the component not to be calculated."
 COMPONENT can be defined as component::version"
   (if (listp component)
       (ergoemacs-theme-component-map-list
-       (or name "list") :map-list (mapcar (lambda(comp) (ergoemacs-theme-get-component comp version)) component)
+       (if name (format "%s" name) "list") :map-list (mapcar (lambda(comp) (ergoemacs-theme-get-component comp version)) component)
        :components component)
     (let* ((comp-name (or (and (symbolp component) (symbol-name component))
                           component))