bug#43252: 27.1; DBus properties lack type hints or overrides

[Michael Albinus <michael.albinus@gmx.de>]

Hugh Daschbach <hugh@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@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@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.