Package: emacs;
Reported by: Hugh Daschbach <hugh <at> ccss.com>
Date: Mon, 7 Sep 2020 00:55:02 UTC
Severity: normal
Found in version 27.1
Done: Michael Albinus <michael.albinus <at> gmx.de>
Bug is archived. No further changes may be made.
Message #121 received at 43252 <at> debbugs.gnu.org (full text, mbox):
From: Michael Albinus <michael.albinus <at> gmx.de> To: Hugh Daschbach <hugh <at> ccss.com> Cc: 43252 <at> debbugs.gnu.org Subject: Re: bug#43252: 27.1; DBus properties lack type hints or overrides Date: Wed, 23 Sep 2020 19:32:08 +0200
Hugh Daschbach <hugh <at> ccss.com> writes:
Hi Hugh,
> I've taken a shot at this. I'm not sure I got the ChangeLog style
> correct. Let me know if I'm still off the beaten path.
Almost. See comment below.
>> `dbus-register' (if SELECTOR is `register') or
>
> Fixed.
Almost. See comment below.
>> Well, this is one approach. Alternatively, we could regard the
>> introspection file as test data, which is located in a file called
>> .../test/lisp/net/dbus-tests/org.gnu.Emacs.TestDBus.xml. This
>> function (the handler for the Introspect method) would read the file,
>> and return its contents.
Oops, I'm mistaken here. The directory shall be called dbus-resources,
see .../test/file-organization.org. Sorry.
> Is:
>
> (let* ((property
> (dbus-introspect-get-property
> :session
> dbus--test-service
> dbus--test-path interface
> property-name))) ...)
>
> preferred over:
>
> (let* ((property
> (dbus-introspect-get-property :session
> dbus--test-service
> dbus--test-path interface property-name)))
> ...)
>
> If not, I'll take another bite at the apple.
I'd vote for the latter, with the first argument :session preceding
the other arguments in the same line. Like
(let* ((property
(dbus-introspect-get-property
:session dbus--test-service
dbus--test-path interface property-name)))
...)
If all arguments fit into 80 columns, do it.
The "canonical" formatting is offered by the `pp' command (pretty
printer). I must confess, that I do not follow all its recommendations.
> Done. ’dbus-annotation-deprecated’. Let me know if you think this
> should be
> ‘dbus--annotation-deprecated’ instead.
No, the former is OK. People might want to use it in their packages.
> In other news, this test:
>
> (should-error
> (dbus-check-arguments :session dbus--test-service :unix-fd 10)
> :type 'dbus-error)
>
> works for me in batch mode, but not interactive mode.
>
> On GNU/Linux, (dired (format "/proc/%s/fd" (emacs-pid))) indicates I
> have 14 open file descriptors on a test instance (emacs -q -Q). On my
> development instance, I've got 27 open file descriptors.
I see. Will investigate.
And now my review for the latest patch versions.
> From be45a75b315e56649fa9e39d199fe5e2b50bf69d Mon Sep 17 00:00:00 2001
> From: Hugh Daschbach <hdasch <at> fastmail.com>
> Date: Tue, 22 Sep 2020 19:36:20 -0700
> Subject: [PATCH 1/2] Add D-Bus property tests.
>
> * test/lisp/net/dbus-tests.el: Add property tests.
> (dbus--test-run-property-test) (dbus--test-property)
> (dbus-test06-property-types): Test property registration, set, get.
If you have one comment for several functions, use only one parenthesis
per line like
(dbus--test-run-property-test, dbus--test-property)
(dbus-test06-property-types): Test property registration, set, get.
> +(defsubst dbus--test-run-property-test (selector name value expected)
> + "Generate a property test: register, set, get, getall sequence.
> +This is a helper function for the macro `dbus--test-property'.
> +The argument SELECTOR indicates whether the test should expand to
> +'dbus-register-property' (if SELECTOR is `register') or
`dbus-register-property' (if SELECTOR is `register') or
> + '((:array
> + :object-path "/node00"
> + :object-path "/node01"
> + :object-path "/node0/node02") .
> + ("/node00" "/node01" "/node0/node02"))
If a cons cell doesn't fit into one line, you shall move the dot "." to
the beginning of the next line, like
(dbus--test-property
"ObjectArray"
'((:array
:object-path "/node00"
:object-path "/node01"
:object-path "/node0/node02")
. ("/node00" "/node01" "/node0/node02"))
I bet the dot "." has the font-lock-warning-face (red foreground color)
in your buffer. Admittedly, it doesn't look prominent :-(
Move the cursor over the misplaced dot; there shall be a help message about.
> + ;; Cleanup.
> + (message "cleanup")
> + (dbus-unregister-service :session dbus--test-service)))
I don't believe we need this message. We see that we're done :-)
> From 3efb1b38d75572b14ac0526dbd79769d6fa89d10 Mon Sep 17 00:00:00 2001
> From: Hugh Daschbach <hdasch <at> fastmail.com>
> Date: Mon, 21 Sep 2020 17:12:49 -0700
> Subject: [PATCH 2/2] Add D-Bus Introspection tests.
>
> * lisp/net/dbus.el (new defconst): D-Bus deprecation name.
>
> * test/lisp/net/dbus-tests.el (dbus--tests-dir)
> (dbus--test-introspect) (dbus--test-examine-interface)
> (dbus--test-validate-annotations) (dbus--test-validate-property)
> (dbus--test-validate-m-or-s) (dbus--test-validate-signal)
> (dbus--test-validate-method) (dbus-test07-introspection): new tests.
That's not true. All but the last functions are new defuns, not new
tests. And please start the explanation with a capital letter, like "New test."
> +(defconst dbus-annotation-deprecated (concat dbus-interface-dbus ".Deprecated")
> + "An annotation value indicating a deprecated interface, method, signal, or property.")
The docstring line does not fit 80 chars. What about
"An annotation indicating a deprecated interface, method, signal, or property."
> +(defvar dbus--tests-dir
> + (file-truename
> + (expand-file-name "dbus-tests"
> + (file-name-directory (or load-file-name
> + buffer-file-name))))
> + "Directory containing test data files.")
As said, it must be "dbus-resources".
> +(ert-deftest dbus-test07-introspection ()
> + :tags '(:expensive-test)
> + "Register an Introspection interface then query it."
:tags must be after the docstring.
> Cheers,
> Hugh
Best regards, Michael.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.