Re: Use lexical-scoping in tests
>> OK, here's a better version. As you can see, it's not nearly as simple. > > Applied onto main + commits addressing all but one FIXME (the one about > `find-file' in org-test.el). Great, thanks, Stefan
Re: Use lexical-scoping in tests
Stefan Monnier writes: >> The fix is in preparation, but obviously I had tested my patch >> incorrectly (i.e. I probably compiled and tested another code than the >> one I had patched). > > OK, here's a better version. As you can see, it's not nearly as simple. Applied onto main + commits addressing all but one FIXME (the one about `find-file' in org-test.el). https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=1a5e3f931c9bbefaefafd4cdcc5f0dfcd1c97769 https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=17b51973bd17f26b4dfa0a5d5f198c7e1c8461dd https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=6074a22bcb25593bd7644b2695d38f8b41200463 https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=e3348ccc03b0aa3e8f8b3d0837b91f8285fe161a -- Ihor Radchenko, Org mode contributor, Learn more about Org mode at https://orgmode.org/. Support Org development at https://liberapay.com/org-mode, or support my work at https://liberapay.com/yantar92
Re: Use lexical-scoping in tests
>> +;; FIXME: `s' is a symbol, so (car-safe s) is always nil. >> +;;(when (eq 'autoload (car-safe s)) >> +;; (unintern s obarray)) >> + > > If I understand correctly, the intended version of this code is supposed > to be > > (when (autoloadp (symbol-function s)) > (unintern s obarray)) > > the idea being "unloading" all the built-in org-related staff. > However, make test will be failing then with byte-compiler error. > I feel that the idea of the code is reasonable, but some detail of how > autoloads work in Emacs is missed. I don't see why you'd need to remove the existing autoloads: they don't specify which directory the file will come from, so if the file still exists in the new Org, the (old) autoload will load from the new Org, as needed. I mean it's OK to remove those autoloads, but really, those are usually harmless and they are the least of your problems. I think the things we'd want/need to "remove" are those things which *aren't* autoloads. Also `unintern` is a fairly powerful operation which can come with undesirable side-effects, so I'd rather replace it with something less risky like `fmakunbound`. >> + ;; FIXME: For the rare cases where we do need to mess with windows, >> + ;; we should let `body' take care of displaying this buffer! >> (setq buffer (find-file file)) > > Could you please elaborate about this fixme? `find-file` displays the buffer in a window. In most uses of this code we don't care whether the buffer is displayed or not, so we should probably use `find-file-noselect` instead. [ As a rule of thumb, most uses of `find-file` (and `switch-to-buffer`) in ELisp code are the result of a misunderstanding from the coder who just uses the commands he's familiar with as a user. But in Elisp, you generally want to use `find-file-noselect` (and `set-buffer`, or maybe `pop-to-buffer`) instead. ] I didn't make the change, tho, because some parts of the tests do care about which buffer is displayed in which window, apparently, so it would take more work. Stefan
Re: Use lexical-scoping in tests
Stefan Monnier writes: >> The fix is in preparation, but obviously I had tested my patch >> incorrectly (i.e. I probably compiled and tested another code than the >> one I had patched). > > OK, here's a better version. As you can see, it's not nearly as simple. Thanks! This new version passes all the tests on my side. However, I have a few questions about FIXME items. > + ;; FIXME: `s' is a symbol, so (car-safe s) is always nil. > + ;;(when (eq 'autoload (car-safe s)) > + ;; (unintern s obarray)) > + If I understand correctly, the intended version of this code is supposed to be (when (autoloadp (symbol-function s)) (unintern s obarray)) the idea being "unloading" all the built-in org-related staff. However, make test will be failing then with byte-compiler error. I feel that the idea of the code is reasonable, but some detail of how autoloads work in Emacs is missed. > +;; FIXME: For the rare cases where we do need to mess with windows, > + ;; we should let `body' take care of displaying this buffer! > (setq buffer (find-file file)) Could you please elaborate about this fixme? -- Ihor Radchenko, Org mode contributor, Learn more about Org mode at https://orgmode.org/. Support Org development at https://liberapay.com/org-mode, or support my work at https://liberapay.com/yantar92
Re: Use lexical-scoping in tests
> The fix is in preparation, but obviously I had tested my patch > incorrectly (i.e. I probably compiled and tested another code than the > one I had patched). OK, here's a better version. As you can see, it's not nearly as simple. Stefan >From 9cd41bcbb6ca6771bd4c79f8b9d07241c67250ea Mon Sep 17 00:00:00 2001 From: Stefan Monnier Date: Wed, 14 Sep 2022 17:21:37 -0400 Subject: [PATCH] testing: Make all files use `lexical-binding` Mainly, add the corresponding cookie, but also add various `require`s so that the compiler knows which vars should be trated as dynbound. This does not fix all the warnings, but does try to eliminate all those about "unused" variables. For the variables truly unused, the patch usually adds an underscore to their name to silence the warning. Some of the fixes affect files which already used `lexical-binding`. Not sure why the test worked before: maybe because the tests were run without compiling them first (which could cause some of the missing `require`d packages to be autoloaded before we got to the problematic code, thus hiding the problem)? I found some suspicious code, for which I added FIXMEs. There are also a few changes to the main files. * lisp/org-protocol.el (org-protocol-check-filename-for-protocol): Don't call `server-edit` if it's not yet defined. [ Needed to get the tests to pass. ] * lisp/ob-core.el (org-babel-temporary-directory) (org-babel-temporary-stable-directory): Always define (and use nil if we don't want to create a directory for it). Simplify the code based on the fact that (defvar V E) only evaluates E if V is not yet `boundp`. (org-babel-temp-file, org-babel-temp-stable-file) (org-babel-remove-temporary-directory) (org-babel-remove-temporary-stable-directory): Adjust accordingly. * lisp/org.el (org-log-beginning): Add FIXME. * testing/org-test.el: Require `org` and `org-id`. (org-id-locations-file): Don't `defconst` it. (org-test-at-id, org-test-in-example-file, org-test-at-marker) (org-test-with-temp-text, org-test-with-temp-text-in-file): Move edebug specs into `declare` (and simplify them). (org-test-with-tramp-remote-dir--worker): Declare dynbound tramp vars. (org--compile-when): Fix quoting of `exp`. (org-test-load): Tweak regexps. * testing/org-batch-test-init.el: Tweak regexp, remove dead code and add a FIXME about it. * testing/lisp/test-ox.el: Require `ox` instead of erroring out if it's not already loaded. Also require `org-inlinetask`. (org-test-with-parsed-data): Silence warnings when `info` is not used. (test-org-export/bind-keyword): Add FIXME. * testing/lisp/test-ox-publish.el: Require `org-test` and `ox-publish`. (test-org-publish/resolve-external-link): Expose lambdas to the compiler. Remove unused var `ids`. (test-org-publish/get-project-from-filename): Remove unused var `file`. * testing/lisp/test-org.el: Require `org-macs`, `org`, `org-inlinetask`, `org-refile`, and `org-agenda`. (test-org/org-read-date): Declare `org-time-was-given` as dynbound. (test-org/set-regexps-and-options): Add FIXME. * testing/lisp/test-org-timer.el: Require `org-timer`. * testing/lisp/test-org-table.el: Require `ox`. * testing/lisp/test-org-protocol.el: Require `org-protocol` instead of erroring out if it's not already loaded. Also require `capture`, and add missing `provide` statement. * testing/lisp/test-org-pcomplete.el: Require `org`. * testing/lisp/test-org-list.el: Require `org-list` and `org`. * testing/lisp/test-org-lint.el: Require `org-footnote` and `org-lint`. * testing/lisp/test-org-footnote.el: Require `org-footnote`. * testing/lisp/test-org-element.el: Require `org-element` instead of erroring out if it's not already loaded. Also require `org` and `org-inlinetask`. * testing/lisp/test-org-duration.el: Require `org-duration`. * testing/lisp/test-org-datetree.el: Require `org-datetree`. * testing/lisp/test-org-colview.el: Require `org-colview`, `org-duration`, and `org-inlinetask`. * testing/lisp/test-org-clock.el: Require `org-duration` and `org-clock`. * testing/lisp/test-org-archive.el: Require `org-archive`. * testing/lisp/test-org-agenda.el (test-org-agenda/bulk-custom-arg-func): Add FIXME. * testing/lisp/test-ol.el: Require `ol` and `org-id`. (test-org-link/store-link): Declare `org-store-link-props` and add FIXME. * testing/lisp/test-oc.el (test-org-cite/export-capability): Add FIXME. * testing/lisp/test-ob.el: Require `ob-core`, `org-src`, `ob-ref`, and `org-table`. (test-ob/eval-header-argument): Rename `foo` to `test-ob--foo` and declare it as dynbound. (test-ob/blocks-with-spaces, test-ob/specific-colnames): Add FIXME. (test-ob/noweb-expansions-in-cache): Declare `noweb-expansions-in-cache-var` as dynbound. * testing/lisp/test-ob-tangle.el: Require `org` and `ob-tangle`. * testing/lisp/test-ob-shell.el: * testing/lisp/test-ob-python.el: Require `ob-core`. * testing/lisp/test-ob-lob.el: Require `ob-lob`. (temporary-value-for-test): Declare as dynbound. * testing/lisp/test-ob-plantuml.e
Re: Use lexical-scoping in tests
Ihor Radchenko [2022-09-14 20:32:53] wrote: > Stefan Monnier writes: >> The patch below simply enables `lexical-binding` in all the test files. >> As far as I can tell, it's all that's needed (beside a missing >> `require`). > Thanks, but the patch causes 23 tests to fail (running make test). Oh, lord! The fix is in preparation, but obviously I had tested my patch incorrectly (i.e. I probably compiled and tested another code than the one I had patched). [ At least it explains why I was seeing this weird "it's all that's needed", which I always hope for but is so rarely true. ] > Also, is there any reason why it was a plain diff? Lack of inspiration, mostly. Stefan
Re: Use lexical-scoping in tests
Stefan Monnier writes: > The patch below simply enables `lexical-binding` in all the test files. > As far as I can tell, it's all that's needed (beside a missing > `require`). Thanks, but the patch causes 23 tests to fail (running make test). Also, is there any reason why it was a plain diff? -- Ihor Radchenko, Org mode contributor, Learn more about Org mode at https://orgmode.org/. Support Org development at https://liberapay.com/org-mode, or support my work at https://liberapay.com/yantar92
Use lexical-scoping in tests
The patch below simply enables `lexical-binding` in all the test files. As far as I can tell, it's all that's needed (beside a missing `require`). Stefan diff --git a/testing/examples/babel.el b/testing/examples/babel.el index a7bb0ccf52..cbd522e243 100644 --- a/testing/examples/babel.el +++ b/testing/examples/babel.el @@ -1,3 +1,4 @@ +;; -*- lexical-binding: t; -*- (string-match-p "^#[[:digit:]]+$" "#123") ;; [[id:73115FB0-6565-442B-BB95-50195A499EF4][detangle:1]] diff --git a/testing/lisp/test-ob-C.el b/testing/lisp/test-ob-C.el index 5f1dd1acb7..793ca31684 100644 --- a/testing/lisp/test-ob-C.el +++ b/testing/lisp/test-ob-C.el @@ -1,4 +1,4 @@ -;;; test-ob-C.el --- tests for ob-C.el +;;; test-ob-C.el --- tests for ob-C.el -*- lexical-binding: t; -*- ;; Copyright (c) 2010-2014, 2019 Sergey Litvinov, Thierry Banel ;; Authors: Sergey Litvinov, Thierry Banel diff --git a/testing/lisp/test-ob-R.el b/testing/lisp/test-ob-R.el index 1e60ae903f..98d73dea91 100644 --- a/testing/lisp/test-ob-R.el +++ b/testing/lisp/test-ob-R.el @@ -1,4 +1,4 @@ -;;; test-ob-R.el --- tests for ob-R.el +;;; test-ob-R.el --- tests for ob-R.el -*- lexical-binding: t; -*- ;; Copyright (c) 2011-2014, 2019 Eric Schulte ;; Authors: Eric Schulte diff --git a/testing/lisp/test-ob-awk.el b/testing/lisp/test-ob-awk.el index 21151b21fc..874d2c0268 100644 --- a/testing/lisp/test-ob-awk.el +++ b/testing/lisp/test-ob-awk.el @@ -1,4 +1,4 @@ -;;; test-ob-awk.el --- tests for ob-awk.el +;;; test-ob-awk.el --- tests for ob-awk.el -*- lexical-binding: t; -*- ;; Copyright (c) 2010-2014, 2019 Sergey Litvinov ;; Authors: Sergey Litvinov diff --git a/testing/lisp/test-ob-clojure.el b/testing/lisp/test-ob-clojure.el index 609fc0ea85..3a21498b4a 100644 --- a/testing/lisp/test-ob-clojure.el +++ b/testing/lisp/test-ob-clojure.el @@ -1,4 +1,4 @@ -;;; test-ob-clojure.el +;;; test-ob-clojure.el -*- lexical-binding: t; -*- ;; Copyright (c) 2018-2022 Free Software Foundation, Inc. ;; Authors: stardiviner diff --git a/testing/lisp/test-ob-emacs-lisp.el b/testing/lisp/test-ob-emacs-lisp.el index e7b85d5ba5..c7bf17784b 100644 --- a/testing/lisp/test-ob-emacs-lisp.el +++ b/testing/lisp/test-ob-emacs-lisp.el @@ -1,4 +1,4 @@ -;;; test-ob-emacs-lisp.el +;;; test-ob-emacs-lisp.el -*- lexical-binding: t; -*- ;; Copyright (c) 2012-2022 Free Software Foundation, Inc. ;; Authors: Eric Schulte, Martyn Jago diff --git a/testing/lisp/test-ob-eshell.el b/testing/lisp/test-ob-eshell.el index d74742690b..4430a21c7a 100644 --- a/testing/lisp/test-ob-eshell.el +++ b/testing/lisp/test-ob-eshell.el @@ -1,4 +1,4 @@ -;;; test-ob-eshell.el +;;; test-ob-eshell.el -*- lexical-binding: t; -*- ;; Copyright (c) 2018 stardiviner ;; Authors: stardiviner diff --git a/testing/lisp/test-ob-exp.el b/testing/lisp/test-ob-exp.el index 1289745aea..b85db33401 100644 --- a/testing/lisp/test-ob-exp.el +++ b/testing/lisp/test-ob-exp.el @@ -1,4 +1,4 @@ -;;; test-ob-exp.el +;;; test-ob-exp.el -*- lexical-binding: t; -*- ;; Copyright (c) 2010-2015, 2019 Eric Schulte ;; Authors: Eric Schulte diff --git a/testing/lisp/test-ob-fortran.el b/testing/lisp/test-ob-fortran.el index 79a35d58e8..3f821e4dac 100644 --- a/testing/lisp/test-ob-fortran.el +++ b/testing/lisp/test-ob-fortran.el @@ -1,4 +1,4 @@ -;;; test-ob-fortran.el --- tests for ob-fortran.el +;;; test-ob-fortran.el --- tests for ob-fortran.el -*- lexical-binding: t; -*- ;; Copyright (c) 2010-2014, 2019 Sergey Litvinov ;; Authors: Sergey Litvinov diff --git a/testing/lisp/test-ob-header-arg-defaults.el b/testing/lisp/test-ob-header-arg-defaults.el index 1106892d55..beb3ea5b8a 100644 --- a/testing/lisp/test-ob-header-arg-defaults.el +++ b/testing/lisp/test-ob-header-arg-defaults.el @@ -1,4 +1,4 @@ -;;; test-ob-header-arg-defaults.el --- tests for default header args from properties +;;; test-ob-header-arg-defaults.el --- tests for default header args from properties -*- lexical-binding: t; -*- ;; Copyright (c) 2013, 2014, 2019 Achim Gratz ;; Authors: Achim Gratz diff --git a/testing/lisp/test-ob-java.el b/testing/lisp/test-ob-java.el index 56ced23740..1a79fc66a1 100644 --- a/testing/lisp/test-ob-java.el +++ b/testing/lisp/test-ob-java.el @@ -1,4 +1,4 @@ -;;; test-ob-java.el --- tests for ob-java.el +;;; test-ob-java.el --- tests for ob-java.el -*- lexical-binding: t; -*- ;; Copyright (c) 2020-2022 Free Software Foundation, Inc. ;; Authors: Eric Schulte diff --git a/testing/lisp/test-ob-julia.el b/testing/lisp/test-ob-julia.el index f6d21726aa..b25235f5fa 100644 --- a/testing/lisp/test-ob-julia.el +++ b/testing/lisp/test-ob-julia.el @@ -1,4 +1,4 @@ -;;; test-ob-python.el --- tests for ob-python.el +;;; test-ob-python.el --- tests for ob-python.el -*- lexical-binding: t; -*- ;; Copyright (c) 2011-2014, 2019, 2021 Eric Schulte ;; Authors: Pedro Bruel, based on test-ob-python.el by Eric Schulte diff --git a/testing/lisp/test-ob-lilypond.el b/testing/lisp/test-ob-lilypond.el index 1c1d73309e..2c76
