Make opts optional for with-<driver> macros (#458)

This makes the advantage etaoin.api2 versions maybe less obvious, but they remain for those who like them. The user guide won't explicitly mention them with the thought that etaoin.api versions are now good enough. Updated clj-kondo macro hooks accordingly. Added some with-driver tests to verify that all this macro sugar works. Also: for consistency renamed arg names: opt and options -> opts. Closes #453
clj-commons · Jun 20, 2022 · d3f06bd · d3f06bd
1 parent d7a95db
commit d3f06bd
Show file tree

Hide file tree

Showing 10 changed files with 512 additions and 232 deletions.
diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc
@@ -13,6 +13,7 @@ Minor Breaking Changes
 The symbol `num-.` is technically an invalid Clojure symbol and can confuse tooling. +
 A grep.app for `num-.` found Etaoin itself as the only user of this var.
 If your code uses `etaoin.keys/num-.`, you'll need to rename it to `etaoin.keys/num-dot`.
+
 * https://github.com/clj-commons/etaoin/issues/430[#430]: Declare the public API.
 We made what we think is a good guess at what the public Etaoin API is.
 The following namespaces are now considered internal and subject to change:
@@ -92,6 +93,9 @@ Other Changes
 * https://github.com/clj-commons/etaoin/issues/380[#380]: Etaoin is now Babashka compatible!
 * https://github.com/clj-commons/etaoin/issues/413[#413]: Etaoin now exports a clj-kondo config to help with the linting of its many handy macros
 * https://github.com/clj-commons/etaoin/pull/357[#357]: Add support for connecting to a remote WebDriver via `:webdriver-url` (thanks https://github.com/verma[@verma] for the PR and https://github.com/mjmeintjes[@mjmeintjes] for the example usage!)
+* https://github.com/clj-commons/etaoin/issues/453[#453]: The `etaoin.api/with-<browser>` macros no longer require `opts` to be specified.
+This makes the advantage of newer `etaoin.api2/with-<browser>` macros maybe less obvious.
+That said, for Etaoin users who have adopted and prefer the api2 versions, they are still there, but no longer documented in the user guide.
 * https://github.com/clj-commons/etaoin/issues/383[#383]: Drop testing for Safari on Windows, Apple no longer releases Safari for Windows
 * https://github.com/clj-commons/etaoin/issues/388[#388]: Drop testing for PhantomJS, development has long ago stopped for PhantomJS
 * https://github.com/clj-commons/etaoin/issues/387[#387]: No longer testing multiple key modifiers for a single webdriver send keys request

diff --git a/doc/01-user-guide.adoc b/doc/01-user-guide.adoc
@@ -333,7 +333,6 @@ Unless otherwise directed, our examples throughout the rest of this guide will a
 [source,clojure]
 ----
 (require '[etaoin.api :as e]
-         '[etaoin.api2 :as e2]
          '[etaoin.keys :as k]
          '[clojure.java.io :as io])
 
@@ -378,9 +377,7 @@ To prevent that, we recommend the `with-<browser>` macros:
 
 [source,clojure]
 ----
-(require '[etaoin.api2 :as e2])
-
-(e2/with-firefox [driver]
+(e/with-firefox driver
   (doto driver
     (e/go "https://google.com")
     ;; ... your code here
@@ -425,16 +422,14 @@ Let's say we want to create a chrome headless driver:
 (e/quit driver)
 ----
 
-The v2 API has ergonomic `with-<browser>` functions that handle cleanup nicely:
+The `with-<browser>` functions handle cleanup nicely:
 
 [source,clojure]
 ----
-(require '[etaoin.api2 :as e2])
-
-(e2/with-chrome [driver {:headless true}]
+(e/with-chrome {:headless true} driver
   (e/go driver "https://clojure.org"))
 
-(e2/with-chrome-headless [driver]
+(e/with-chrome-headless driver
   (e/go driver "https://clojure.org"))
 ----
 
@@ -1324,7 +1319,7 @@ For example, the default `:normal` strategy:
 
 [source,clojure]
 ----
-(e2/with-chrome [driver]
+(e/with-chrome driver
   (e/go driver sample-page)
   ;; by default you'll hang on this line until the page loads
   ;; (do-something)
@@ -1335,7 +1330,7 @@ Load strategy option of `:none`:
 
 [source,clojure]
 ----
-(e2/with-chrome [driver {:load-strategy :none}]
+(e/with-chrome {:load-strategy :none} driver
   (e/go driver sample-page)
   ;; no pause, no waiting, acts immediately
   ;; (do-something)
@@ -1557,9 +1552,9 @@ To start a driver with devtools support enabled specify a `:dev` map.
 //{:test-doc-blocks/test-ns user-guide-devtools-test}
 [source,clojure]
 ----
-(require '[etaoin.api2 :as e2])
+(require '[etaoin.api :as e])
 
-(e2/with-chrome [driver {:dev {}}]
+(e/with-chrome driver {:dev {}}
   ;; do some stuff
 )
 ----
@@ -2170,12 +2165,12 @@ There are several shortcuts to run Chrome or Firefox in headless mode:
 
 ;; or
 
-(require '[etaoin.api2 :as e2])
+(require '[etaoin.api :as e])
 
-(e2/with-chrome-headless [driver]
+(e/with-chrome-headless driver
   (e/go driver "https://clojure.org"))
 
-(e2/with-firefox-headless [driver {:log-level :all}] ;; extra settings
+(e/with-firefox-headless {:log-level :all} driver ;; notice extra settings
   (e/go driver "https://clojure.org"))
 ----
 
@@ -2184,7 +2179,7 @@ There are also the `when-headless` and `when-not-headless` macros that conditona
 //{:test-doc-blocks/test-ns user-guide-headless-test}
 [source,clojure]
 ----
-(e2/with-chrome [driver]
+(e/with-chrome driver
   (e/when-not-headless driver
     ;;... some actions that might be not available in headless mode
     )
@@ -2229,7 +2224,8 @@ Set a custom `User-Agent` header with the `:user-agent` option when creating a d
 
 [source,clojure]
 ----
-(e2/with-firefox [driver {:user-agent "Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1)"}]
+(e/with-firefox {:user-agent "Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1)"}
+                driver
   (e/get-user-agent driver))
 ;; => "Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1)"
 ----
@@ -2299,10 +2295,10 @@ Example:
 
 ;; Connect to a chrome instance on browserless.io via :webdriver-url
 ;; (replace YOUR-API-TOKEN with a valid browserless.io api token if you want to try this out)
-(e2/with-chrome [driver
-                 {:webdriver-url "https://chrome.browserless.io/webdriver"
-                  :capabilities {"browserless:token" "YOUR-API-TOKEN"
-                                 "chromeOptions" {"args" ["--no-sandbox"]}}}]
+(e/with-chrome {:webdriver-url "https://chrome.browserless.io/webdriver"
+                :capabilities {"browserless:token" "YOUR-API-TOKEN"
+                               "chromeOptions" {"args" ["--no-sandbox"]}}}
+               driver
   (e/go driver "https://en.wikipedia.org/")
   (e/wait-visible driver [{:id :simpleSearch} {:tag :input :name :search}])
   (e/fill driver {:tag :input :name :search} "Clojure programming language")
@@ -2414,13 +2410,12 @@ If for some reason you want to reuse a single driver instance for all tests:
 (ns project.test.integration
   "A module for integration tests"
   (:require [clojure.test :refer [deftest is use-fixtures]]
-            [etaoin.api :as e]
-            [etaoin.api :as e2]))
+            [etaoin.api :as e]))
 
 (def ^:dynamic *driver*)
 
 (defn fixture-browser [f]
-  (e2/with-chrome-headless [driver {:args ["--no-sandbox"]}]
+  (e/with-chrome-headless {:args ["--no-sandbox"]} driver
     (e/disconnect-driver driver)
     (binding [*driver* driver]
       (f))
@@ -2454,7 +2449,7 @@ For faster testing you can use this example:
 .....
 
 (defn fixture-browser [f]
-  (e2/with-chrome-headless [driver {:args ["--no-sandbox"]}]
+  (e/with-chrome-headless {:args ["--no-sandbox"]} driver
     (binding [*driver* driver]
       (f))))
 
@@ -2746,7 +2741,7 @@ Reproduction:: For example, `chromedriver` used to throw an error when calling `
 +
 [source,clojure]
 ----
-(e2/with-chrome [driver]
+(e/with-chrome driver
   (e/maximize driver))
 ;; an exception with "cannot get automation extension" was thrown
 ----
@@ -2851,7 +2846,7 @@ This bypasses OS security model.
 +
 [source,clojure]
 ----
-(e2/with-chrome [driver {:args ["--no-sandbox"]}]
+(e/with-chrome {:args ["--no-sandbox"]} driver
   (e/go driver "https://clojure.org"))
 ----
 

diff --git a/resources/clj-kondo.exports/etaoin/etaoin/config.edn b/resources/clj-kondo.exports/etaoin/etaoin/config.edn
@@ -1,11 +1,15 @@
 {:linters
  {:etaoin/with-x-action {:level :error}
-  :etaoin/binding-sym {:level :error}}
+  :etaoin/binding-sym {:level :error}
+  :etaoin/opts-map-type {:level :error}
+  :etaoin/opts-map-pos {:level :error}
+  :etaoin/empty-opts {:level :warning}}
  :hooks
  {:analyze-call
   {etaoin.api/with-chrome            etaoin.api/with-browser
-   etaoin.api/with-firefox           etaoin.api/with-browser
    etaoin.api/with-chrome-headless   etaoin.api/with-browser
+   etaoin.api/with-firefox           etaoin.api/with-browser
+   etaoin.api/with-firefox-headless  etaoin.api/with-browser
    etaoin.api/with-edge              etaoin.api/with-browser
    etaoin.api/with-edge-headless     etaoin.api/with-browser
    etaoin.api/with-phantom           etaoin.api/with-browser

diff --git a/resources/clj-kondo.exports/etaoin/etaoin/etaoin/api.clj_kondo b/resources/clj-kondo.exports/etaoin/etaoin/etaoin/api.clj_kondo
@@ -2,29 +2,63 @@
   (:require [clj-kondo.hooks-api :as api]
             [etaoin.hooks-util :as h]))
 
-(defn- with-bound-arg [node bound-arg-ndx]
+(defn- nil-node? [n]
+  (and (api/token-node? n) (nil? (api/sexpr n))))
+
+(defn- with-bound-arg [node arg-offset]
   (let [macro-args (rest (:children node))
-        binding-sym (nth macro-args bound-arg-ndx nil)]
-    (if-not (h/symbol-node? binding-sym)
-      ;; could use clj-kondo findings, but I think this is good for now
-      (api/reg-finding! (assoc (if binding-sym
-                                 (meta binding-sym)
-                                 (meta node))
-                               :message (format "Expected binding symbol as %s arg"
-                                                ;; use words instead of numbers to avoid ambiguity
-                                                (case bound-arg-ndx 1 "second" 2 "third"))
+        leading-args (take arg-offset macro-args)
+        interesting-args (drop arg-offset macro-args)
+        [opts binding-sym & body] (if (h/symbol-node? (second interesting-args))
+                                    interesting-args
+                                    (cons nil interesting-args))]
+    ;; if the user has specified nil or {} for options we can suggest that is not necessary
+    (when (and opts
+               (or (and (api/map-node? opts) (not (seq (:children opts))))
+                   (nil-node? opts)))
+      (api/reg-finding! (assoc (meta opts)
+                               :message "Empty or nil driver options can be omitted"
+                               :type :etaoin/empty-opts)))
+
+    (cond
+      (not (h/symbol-node? binding-sym))
+      ;; it makes more sense here to report on the incoming node position instead of what we expect to be the binding-sym
+      (api/reg-finding! (assoc (meta node)
+                               :message "Expected binding symbol for driver"
                                :type :etaoin/binding-sym))
-      (let [leading-args (take bound-arg-ndx macro-args)
-            body (drop (inc bound-arg-ndx) macro-args)]
-        {:node (api/list-node
-                (list*
+
+      ;; we don't want to explicitly expect a map because the map might come from
+      ;; an evalution, but we can do some checks
+      (and opts ;; we'll assume a list-node is a function call (eval)
+           (not (nil-node? opts)) ;; nil is actually old-v1 syntax acceptable
+           (not (api/list-node? opts)) ;; some fn call
+           (not (h/symbol-node? opts)) ;; from a binding maybe
+           ;; there are other eval node types... @(something) for example... maybe we'll add them in if folks ask
+           (not (api/map-node? opts)))
+      ;; we can report directly on the opts node, because at this point we know we expect
+      ;; this arg position to be an opts map
+      (api/reg-finding! (assoc (meta opts)
+                               :message "When specified, opts should be a map"
+                               :type :etaoin/opts-map-type))
+
+      ;; one last nicety, if the first form in body is a map, the user has accidentally swapped
+      ;; binding and opt args
+      (api/map-node? (first body))
+      (api/reg-finding! (assoc (meta (first body))
+                               :message "When specified, opts must appear before binding symbol"
+                               :type :etaoin/opts-map-pos))
+
+      :else
+      {:node (api/list-node
+               (list*
                  (api/token-node 'let)
-                   ;; simulate the effect, macro is creating a new thing (driver for example)
-                   ;; via binding it. I don't think the bound value matters for the linting process
+                 ;; simulate the effect, macro is creating a new thing (driver for example)
+                 ;; via binding it. I don't think the bound value matters for the linting process
                  (api/vector-node [binding-sym (api/map-node [])])
-                   ;; reference the other args so that they are not linted as unused
+                 ;; reference the other args so that they are not linted as unused
                  (api/vector-node leading-args)
-                 body))}))))
+                 opts ;; might be a binding, so ref it too
+                 body))})))
 
 (defn- with-x-down
   "This is somewhat of a maybe an odd duck.
@@ -58,15 +92,15 @@
 
 (defn with-browser
   "Covers etaoin.api/with-chrome and all its variants
-  [opt bind & body]"
+  [opt? bind & body]"
   [{:keys [node]}]
-  (with-bound-arg node 1))
+  (with-bound-arg node 0))
 
 (defn with-driver
   "Very similar to with-browser but bound arg is 1 deeper
-  [type opt bind & body]"
+  [type opt? bind & body]"
   [{:keys [node]}]
-  (with-bound-arg node 2))
+  (with-bound-arg node 1))
 
 (defn with-key-down
   "[input key & body]"

diff --git a/script/test.clj b/script/test.clj
@@ -141,7 +141,7 @@ Notes:
                                 (with-out-str (shell/clojure "-Spath" (str "-A" aliases)))
                                 "--main" "bb-test-runner"]))
               test-runner-args (case test-id
-                                 "api" ["--namespace" "etaoin.api-test"]
+                                 "api" ["--namespace-regex" "etaoin.api.*-test$"]
                                  "ide" ["--namespace" "etaoin.ide-test"]
                                  "unit" ["--namespace-regex" ".*unit.*-test$"]
                                  "all" [])