Package critique: modeline for air quality information

Package critique: modeline for air quality information

[John Haman]

I wrote a package that adds local air quality statistics to the mode-line. Here it is:

https://github.com/jthaman/air-quality/blob/main/air-quality.el

If you are so inclined, I'd like some thoughts on the code. It's short, but I'm trying to get better at Emacs Lisp (at least this week, while I'm on vacation...)

--
Dr. John Haman
Maryland, USA

Re: Package critique: modeline for air quality information

[Eshel Yaron]

Hello John,

John Haman <mail@johnhaman.org> writes:

> I wrote a package that adds local air quality statistics to the
> mode-line.

Cool!

>
> https://github.com/jthaman/air-quality/blob/main/air-quality.el
>
> If you are so inclined, I'd like some thoughts on the code. It's
> short, but I'm trying to get better at Emacs Lisp (at least this week,
> while I'm on vacation...)
>

My suggestion would be to set this up as a global minor mode.  Also, you
may want to define your "public variables" as user options, so they can
be customized with `M-x customize` and friends.  Something along the
lines of:

diff --git a/air-quality.el b/air-quality.el
index 1781515..533ce1b 100644
--- a/air-quality.el
+++ b/air-quality.el
@@ -6,7 +6,7 @@ ;;; air-quality.el --- Air quality modeline indicator and reporting tool  -*- le
 ;; URL: https://github.com/jthaman/air-quality
 ;; Version: 0.1
 ;; Package-Requires: ((emacs "27.1"))
-;; Keywords: bling
+;; Keywords: convenience
 
 ;; This program is free software; you can redistribute it and/or modify
 ;; it under the terms of the GNU General Public License as published by
@@ -21,35 +21,44 @@ ;;; air-quality.el --- Air quality modeline indicator and reporting tool  -*- le
 ;; You should have received a copy of the GNU General Public License
 ;; along with this program.  If not, see <https://www.gnu.org/licenses/>.
 
-;;; Package Imports
-(require 'url)
-(require 'json)
-(require 'cl-lib)
-
-
 ;;; Commentary:
+
 ;; Add information about local air quality to the modeline. Air Quality
 ;; information is downloaded from the Open Weather Map Air Pollution API.
 
-
 ;;; Code:
 
+;;;; Package Imports
+
+(require 'url)
+(require 'json)
+(require 'cl-lib)
+
+;;;; User options
 
-;;; Public Variables
-(defvar air-quality-open-weather-api-key nil
-  "A string. API key for Open Weather Map.")
+(defgroup air-quality nil
+  "Air quality mode-line indicator."
+  :group 'mode-line)
+
+(defcustom air-quality-open-weather-api-key nil
+  "API key for Open Weather Map."
+  :type '(choice (const  :tag "Unset" nil)
+                 (string :tag "Your API key")))
 
 (defvar air-quality-refresh-interval 60
+  ;; XXX - turn into a defcustom
   "An integer. Number of minutes between refreshes of air quality information.")
 
 (defvar air-quality-latitude nil
+  ;; XXX - likewise
   "A float. Your latitude.")
 
 (defvar air-quality-longitude nil
+  ;; XXX - likewise
   "A float. Your longitude.")
 
+;;;; Private Variables
 
-;;; Private Variables
 (defvar air-quality--timer nil)
 
 (defvar air-quality--co nil "Carbon Monoxide level (micrograms per cubic-meter).")
@@ -62,30 +71,44 @@ (defvar air-quality--pm10 nil "PM 10 level (micrograms per cubic-meter).")
 (defvar air-quality--nh3 nil "Ammonia level (micrograms per cubic-meter).")
 (defvar air-quality--level nil "Overall Air Quality (AQI).")
 
-(defun air-quality--make-forecast-call (key lat lon)
-  "Create an API request for the future forecast of Air Quality information from Open Weather Map."
-  (concat "http://api.openweathermap.org/data/2.5/air_pollution/forecast?lat="
-          (number-to-string lat)
-          "&lon="
-          (number-to-string lon)
-          "&appid="
-          key))
-
-(defun air-quality--make-api-call (key lat lon)
-  "Create an API request to Open Weather Map."
-  (concat "http://api.openweathermap.org/data/2.5/air_pollution?lat="
-          (number-to-string lat)
-          "&lon="
-          (number-to-string lon)
-          "&appid="
-          key))
-
-(defvar air-quality--index-alist '((1 . "Good")
-                                   (2 . "Fair")
-                                   (3 . "Moderate")
-                                   (4 . "Poor")
-                                   (5 . "Very Poor")))
-
+(defun air-quality--make-api-call (key latitude longitude)
+  "Return Open Weather Map URL for LATITUDE and LONGITUDE with API key KEY."
+  (url-parse-make-urlobj
+   "https" nil nil "api.openweathermap.org" nil
+   (concat "/data/2.5/air_pollution/?"
+           (url-build-query-string `(("lat" ,latitude)
+                                     ("lon" ,longitude)
+                                     ("appid" ,key))))))
+
+(defconst air-quality--index-alist '((1 . "Good")
+                                     (2 . "Fair")
+                                     (3 . "Moderate")
+                                     (4 . "Poor")
+                                     (5 . "Very Poor")))
+
+(defvar air-quality-indicator
+  '(:eval
+    (propertize (concat "  " (alist-get air-quality--level
+                                        air-quality--index-alist))
+                'face 'mode-line-buffer-id
+                'help-echo (format "Carbon Monoxide: %d µg/m³
+Nitrogen Oxide: %d µg/m³
+Nitrogen Dioxide: %d µg/m³
+Ozone: %d µg/m³
+Sulfur Dioxide: %d µg/m³
+PM 2.5: %d µg/m³
+PM 10: %d µg/m³
+Ammonia: %d µg/m³"
+                                   air-quality--co
+                                   air-quality--no
+                                   air-quality--no2
+                                   air-quality--o3
+                                   air-quality--so2
+                                   air-quality--pm2_5
+                                   air-quality--pm10
+                                   air-quality--nh3)
+                'mouse-face 'mode-line-highlight)))
+(put 'air-quality-indicator 'risky-local-variable t)
 
 (defun air-quality--get-update ()
   "Query Open Weather for air quality information."
@@ -93,7 +116,7 @@ (defun air-quality--get-update ()
    (air-quality--make-api-call air-quality-open-weather-api-key
                                air-quality-latitude
                                air-quality-longitude)
-   (lambda (events)
+   (lambda (_events)
      (goto-char url-http-end-of-headers)
      (let ((json-object-type 'plist)
            (json-key-type 'symbol)
@@ -108,57 +131,24 @@ (defun air-quality--get-update ()
          (setq air-quality--pm2_5 (plist-get components 'pm2_5))
          (setq air-quality--pm10 (plist-get components 'pm10))
          (setq air-quality--nh3 (plist-get components 'nh3))
-         (setq air-quality--level (cadadr (aref (plist-get result 'list) 0)))
-         (air-quality--set-indicator)
-         (run-with-idle-timer 1 nil #'air-quality--append-modeline)
-         )))))
-
-(defun air-quality--set-indicator ()
-  (defvar-local air-quality-indicator
-      (list (propertize (concat "  " (alist-get air-quality--level air-quality--index-alist))
-                        'face 'mode-line-buffer-id
-                        'help-echo (purecopy (format "Carbon Monoxide: %d µg/m³
-Nitrogen Oxide: %d µg/m³
-Nitrogen Dioxide: %d µg/m³
-Ozone: %d µg/m³
-Sulfur Dioxide: %d µg/m³
-PM 2.5: %d µg/m³
-PM 10: %d µg/m³
-Ammonia: %d µg/m³"
-                                                     air-quality--co
-                                                     air-quality--no
-                                                     air-quality--no2
-                                                     air-quality--o3
-                                                     air-quality--so2
-                                                     air-quality--pm2_5
-                                                     air-quality--pm10
-                                                     air-quality--nh3))
-                        'mouse-face 'mode-line-highlight
-                        )))
-  (put 'air-quality-indicator 'risky-local-variable t))
-
-
-(defun air-quality--append-modeline ()
-  (unless (cl-find '(:eval air-quality-indicator) mode-line-format :test 'equal)
-    (setq-default mode-line-format
-                  (reverse (append '((:eval air-quality-indicator)) (reverse mode-line-format))))))
+         (setq air-quality--level (cadadr (aref (plist-get result 'list) 0))))))))
 
 ;;;###autoload
-(defun air-quality-setup-modeline ()
-
-  ;; This could be better...
-  ;; if no timer, start one
-  (if (null air-quality--timer)
-      (setq air-quality--timer
-            (run-with-timer 0 (* 60 air-quality-refresh-interval) #'air-quality--get-update))
-    ;; otherwise, cancel the timer and start one
-    (progn
-      (cancel-timer air-quality--timer)
-      (setq air-quality--timer
-            (run-with-timer 0 (* 60 air-quality-refresh-interval) #'air-quality--get-update))
-      )))
-
+(define-minor-mode air-quality-mode
+  "Minor mode for displaying air quality information in the mode line."
+  :group 'air-quality
+  :global t
+  (if air-quality-mode
+      (progn
+        ;; XXX - check that air-quality-open-weather-api-key,
+        ;; air-quality-latitude and air-quality-longitude are set
+        (add-to-list 'mode-line-misc-info 'air-quality-indicator t )
+        (setq air-quality--timer
+              (run-with-timer 0 (* 60 air-quality-refresh-interval)
+                              #'air-quality--get-update)))
+    (setq mode-line-misc-info (delq 'air-quality-indicator mode-line-misc-info))
+    (cancel-timer air-quality--timer))
+  (force-mode-line-update))
 
 (provide 'air-quality)
-
 ;;; air-quality.el ends here

> --
> Dr. John Haman
> Maryland, USA

Re: Package critique: modeline for air quality information

[John Haman]

Thank you for the fast and thorough response!

This definitely makes the package better. I applied the patch.

I had some 404 issue with the new version of `air-quality--make-api-call', so I reverted back to the old one that just uses a simple string concatenation. Is that OK? Or is there some specific advantage to using `url-parse-make-urlobj' ?

https://github.com/jthaman/air-quality

You might check to see if I implemented the variable checking in a correct way in the definition of `air-quality-mode'.

-John

--
Dr. John Haman
Maryland, USA

Re: Package critique: modeline for air quality information

[Eshel Yaron]

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

John Haman <mail@johnhaman.org> writes:

>
> I had some 404 issue with the new version of
> `air-quality--make-api-call', so I reverted back to the old one that
> just uses a simple string concatenation. Is that OK?

Sure,

> Or is there some specific advantage to using `url-parse-make-urlobj' ?

Not that I know of, I just find it a bit cleaner.

>
> https://github.com/jthaman/air-quality
>
> You might check to see if I implemented the variable checking in a correct way in the definition of `air-quality-mode'.
>

Looks good, a possible improvement in that area would be to prompt the
user for these settings when they try to activate the mode, instead of
silently failing.  Also note that for the `defcustom`s, if you set the
`:type` to `number` then the default value `nil` isn't of the right
type.  You can see this mismatch indicated in the Custom buffer, for
example.

Here are two patches to make these suggestions concrete, feel free to
apply them as they are, ignore them completely, or anything in between:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Refine-defcustom-types.patch --]
[-- Type: text/x-patch, Size: 1198 bytes --]

From 0b6f014db9d275d6ea6f3b1e04d3389018fd1705 Mon Sep 17 00:00:00 2001
From: Eshel Yaron <me@eshelyaron.com>
Date: Fri, 1 Sep 2023 13:40:34 +0200
Subject: [PATCH 1/2] Refine 'defcustom' types

---
 air-quality.el | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/air-quality.el b/air-quality.el
index 8f55100..7895b8c 100644
--- a/air-quality.el
+++ b/air-quality.el
@@ -45,16 +45,18 @@ (defcustom air-quality-open-weather-api-key nil
                  (string :tag "Your API key")))
 
 (defcustom air-quality-refresh-interval 60
-  "An integer. Number of minutes between refreshes of air quality information."
-  :type 'number)
+  "Number of minutes between refreshes of air quality information."
+  :type 'integer)
 
 (defcustom air-quality-latitude nil
-  "A float. Your latitude."
-  :type 'number)
+  "Your latitude."
+  :type '(choice (const   :tag "Unset" nil)
+                 (integer :tag "Your latitude")))
 
 (defcustom air-quality-longitude nil
-  "A float. Your longitude."
-  :type 'number)
+  "Your longitude."
+  :type '(choice (const   :tag "Unset" nil)
+                 (integer :tag "Your longitude")))
 
 ;;;; Private Variables
 
-- 
2.42.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: attachment --]
[-- Type: text/x-patch, Size: 2190 bytes --]

From bc4c729ab26e34515cbc9d90ca182af4a8939a00 Mon Sep 17 00:00:00 2001
From: Eshel Yaron <me@eshelyaron.com>
Date: Fri, 1 Sep 2023 13:42:22 +0200
Subject: [PATCH 2/2] Prompt for missing settings

---
 air-quality.el | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/air-quality.el b/air-quality.el
index 7895b8c..c6d2250 100644
--- a/air-quality.el
+++ b/air-quality.el
@@ -144,14 +144,27 @@ (define-minor-mode air-quality-mode
   :group 'air-quality
   :global t
   (if air-quality-mode
-      (progn
-        ;; Check that air-quality-open-weather-api-key,
-        ;; air-quality-latitude and air-quality-longitude are set
-        (when (and air-quality-open-weather-api-key air-quality-latitude air-quality-longitude)
-          (add-to-list 'mode-line-misc-info 'air-quality-indicator t )
-          (setq air-quality--timer
-                (run-with-timer 0 (* 60 air-quality-refresh-interval)
-                                #'air-quality--get-update))))
+      (let ((need-save nil))
+        (unless air-quality-open-weather-api-key
+          (setq air-quality-open-weather-api-key
+                (read-string "Set your Open Weather API key: ")
+                need-save t)
+          (customize-mark-to-save 'air-quality-open-weather-api-key))
+        (unless air-quality-latitude
+          (setq air-quality-latitude
+                (read-number "Set your latitude: ")
+                need-save t)
+          (customize-mark-to-save 'air-quality-latitude))
+        (unless air-quality-longitude
+          (setq air-quality-longitude
+                (read-number "Set your longitude: ")
+                need-save t)
+          (customize-mark-to-save 'air-quality-longitude))
+        (when need-save (custom-save-all))
+        (add-to-list 'mode-line-misc-info 'air-quality-indicator t )
+        (setq air-quality--timer
+              (run-with-timer 0 (* 60 air-quality-refresh-interval)
+                              #'air-quality--get-update)))
     (setq mode-line-misc-info (delq 'air-quality-indicator mode-line-misc-info))
     (cancel-timer air-quality--timer))
   (force-mode-line-update))
-- 
2.42.0


[-- Attachment #4: Type: text/plain, Size: 63 bytes --]


Best,

Eshel

> -John
>
> --
> Dr. John Haman
> Maryland, USA

Re: Package critique: modeline for air quality information

[John Haman]

Eshel Yaron <me@eshelyaron.com> writes:

>
> Here are two patches to make these suggestions concrete, feel free to
> apply them as they are, ignore them completely, or anything in between:

These are nice! Both patches applied.

-- 
Dr. John Haman
Maryland, USA

Re: Package critique: modeline for air quality information

[tpeplt]

The following message is a courtesy copy of an article
that has been posted to gnu.emacs.help as well.

John Haman <mail@johnhaman.org> writes:

> I wrote a package that adds local air quality statistics to the
> mode-line. Here it is:
>
> https://github.com/jthaman/air-quality/blob/main/air-quality.el
>
> If you are so inclined, I'd like some thoughts on the code. It's
> short, but I'm trying to get better at Emacs Lisp (at least this week,
> while I'm on vacation...)
>
> --

Some good practices to follow:

1. Byte-compile the source to locate and resolve many types of problems.
Byte compilation is available as a command (‘emacs-lisp-byte-compile’)
or via the Emacs-Lisp menu when editing a .el file.  With your
‘air-quality.el’ (version 0.1 from the git repository), it yields the
following message:

> In air-quality--get-update:
> air-quality.el:125:17: Warning: reference to free variable
> ‘url-http-end-of-headers’

2. Emacs Lisp also has a ‘lint’ available via the Emacs-Lisp menu.  This
can be helpful, but it can also find spurious (non-existent) problems
that are due to its limitations.  For ‘air-quality.el’, it yields:

> In function air-quality--get-update:
> air-quality.el:118:Warning: Reference to unbound symbol:
> url-http-end-of-headers
>
> Linting finished.

3. ‘Checkdoc’ can help identify good practices, such as doc-string
standards, so that you (and others) will have usable descriptions of
procedures and data structures.  It looks as though you have run it on
air-quality.el because it gives no warnings or errors.

4. Consider adding the code as an ELPA package that can be installed via
the Emacs menu Options/Manage Emacs Packages (or the command
‘list-packages’) once it has reached a level of maturity that you
consider acceptable.

--

Re: Package critique: modeline for air quality information

[Emanuel Berg]

John Haman wrote:

> I wrote a package that adds local air quality statistics to
> the mode-line. Here it is:
>
> https://github.com/jthaman/air-quality/blob/main/air-quality.el
>
> If you are so inclined, I'd like some thoughts on the code.
> It's short, but I'm trying to get better at Emacs Lisp (at
> least this week, while I'm on vacation...)

Can you yank the source here as well or provide a URL to it,
that works without JS?

But in general, if you byte-compile the source, then
native-compile it, and do (checkdoc-current-buffer t) and
remove all the warnings, then the Elisp should be good enough
and you will improve in the process.

-- 
underground experts united
https://dataswamp.org/~incal

Re: Package critique: modeline for air quality information

[John Haman]

Emanuel Berg <incal@dataswamp.org> writes:
>
> Can you yank the source here as well or provide a URL to it,
> that works without JS?
>

Here is a link without JS (does your web browser not have JS??)

https://raw.githubusercontent.com/jthaman/air-quality/main/air-quality.el


-- 
Dr. John Haman
Maryland, USA

Re: Package critique: modeline for air quality information

[Emanuel Berg]

John Haman wrote:

>> Can you yank the source here as well or provide a URL to
>> it, that works without JS?
>
> Here is a link without JS (does your web browser not have
> JS??)

Not Emacs-w3m which I use the most, I of course have Firefox
as well but it isn't integrated in the pleasant computer use
workflow, if you will.

> https://raw.githubusercontent.com/jthaman/air-quality/main/air-quality.el

Thanks, looks clean!

-- 
underground experts united
https://dataswamp.org/~incal