bug#23357: [PATCH] Don't let `css--property-values' return duplicates

bug#23357: [PATCH] Don't let `css--property-values' return duplicates

[Simen Heggestøyl]

[-- Attachment #1.1: Type: text/plain, Size: 337 bytes --]

By a recent discussion on emacs-devel I was made aware that
completion-at-point functions are themselves responsible for removing
duplicate entries in their results.

The attached patch ensures that `css--property-values' doesn't return
duplicate values as it would otherwise do sometimes, along with a
regression test for it.

-- Simen

[-- Attachment #1.2: Type: text/html, Size: 435 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Don-t-let-css-property-values-return-duplicates.patch --]
[-- Type: text/x-patch, Size: 3458 bytes --]

From 83ada4ea1fdd8329d5383301dca25839cf46c827 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Simen=20Heggest=C3=B8yl?= <simenheg@gmail.com>
Date: Sun, 24 Apr 2016 11:03:34 +0200
Subject: [PATCH] Don't let `css--property-values' return duplicates

* lisp/textmodes/css-mode.el (css--property-values): Don't return
duplicate values.

* test/lisp/textmodes/css-mode-tests.el (css-test-property-values):
Take the above into account.
(css-test-property-values-no-duplicates): Test that duplicates aren't
returned by `css--property-values'.
---
 lisp/textmodes/css-mode.el            | 15 ++++++++-------
 test/lisp/textmodes/css-mode-tests.el | 18 ++++++++++++++----
 2 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/lisp/textmodes/css-mode.el b/lisp/textmodes/css-mode.el
index 2a61fe3..792d482 100644
--- a/lisp/textmodes/css-mode.el
+++ b/lisp/textmodes/css-mode.el
@@ -792,13 +792,14 @@ css--property-values
 Completion candidates are looked up in `css-property-alist' by
 the string PROPERTY."
   (or (gethash property css--property-value-cache)
-      (seq-mapcat
-       (lambda (value)
-         (if (stringp value)
-             (list value)
-           (or (css--value-class-lookup value)
-               (css--property-values (symbol-name value)))))
-       (cdr (assoc property css-property-alist)))))
+      (seq-uniq
+       (seq-mapcat
+        (lambda (value)
+          (if (stringp value)
+              (list value)
+            (or (css--value-class-lookup value)
+                (css--property-values (symbol-name value)))))
+        (cdr (assoc property css-property-alist))))))
 
 (defun css--complete-property-value ()
   "Complete property value at point."
diff --git a/test/lisp/textmodes/css-mode-tests.el b/test/lisp/textmodes/css-mode-tests.el
index 9c5953d..ee5705f 100644
--- a/test/lisp/textmodes/css-mode-tests.el
+++ b/test/lisp/textmodes/css-mode-tests.el
@@ -24,8 +24,9 @@
 
 ;;; Code:
 
-(require 'ert)
 (require 'css-mode)
+(require 'ert)
+(require 'seq)
 
 (ert-deftest css-test-property-values ()
   ;; The `float' property has a flat value list.
@@ -36,9 +37,10 @@
   ;; The `list-style' property refers to several other properties.
   (should
    (equal (sort (css--property-values "list-style") #'string-lessp)
-          (sort (append (css--property-values "list-style-type")
-                        (css--property-values "list-style-position")
-                        (css--property-values "list-style-image"))
+          (sort (seq-uniq
+                 (append (css--property-values "list-style-type")
+                         (css--property-values "list-style-position")
+                         (css--property-values "list-style-image")))
                 #'string-lessp)))
 
   ;; The `position' property is tricky because it's also the name of a
@@ -57,6 +59,14 @@
   ;; because it refers to the value class of the same name.
   (should (= (length (css--property-values "color")) 18)))
 
+(ert-deftest css-test-property-values-no-duplicates ()
+  "Test that `css--property-values' returns no duplicates."
+  ;; The `flex' property is prone to duplicate values; if they aren't
+  ;; removed, it'll contain at least two instances of `auto'.
+  (should
+   (equal (sort (css--property-values "flex") #'string-lessp)
+          '("auto" "content" "none"))))
+
 (ert-deftest css-test-value-class-lookup ()
   (should
    (equal (sort (css--value-class-lookup 'position) #'string-lessp)
-- 
2.8.0.rc3

bug#23357: [PATCH] Don't let `css--property-values' return duplicates

[Stefan Monnier]

> By a recent discussion on emacs-devel I was made aware that
> completion-at-point functions are themselves responsible for removing
> duplicate entries in their results.

No they're not.  It's the UI code (the code that calls
completion-at-point-function) which is responsible for the removal
of duplicates.

This said, removal of duplicates within the completion table, like your
patch does, is harmless, so this is not an objection against your patch.


        Stefan

bug#23357: [PATCH] Don't let `css--property-values' return duplicates

[Simen Heggestøyl]

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

On Sun, Apr 24, 2016 at 4:51 PM, Stefan Monnier 
<monnier@iro.umontreal.ca> wrote:
> No they're not.  It's the UI code (the code that calls
> completion-at-point-function) which is responsible for the removal
> of duplicates.

Ah, after rereading the conversation I see that I misunderstood it the
first time.

> This said, removal of duplicates within the completion table, like 
> your
> patch does, is harmless, so this is not an objection against your 
> patch.

OK, then I still find it sensible to tidy up duplicates here. Patch
installed!

-- Simen

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