Repository: JohnPaton/airbase

Issue:
Drop "pl"/"shortpl" terminology in favor of something more readable
This terminology is only really used internally by the portal for generating URLs, it doesn't need to be exposed to the user. 

The CSVs themselves use `AirPollutant` for the name (now: `pl`), `AirPollutantCode` for the URL containing the id (now: `shortpl`), and the portal refers to the "pollutant id" in the text.

The [data dictionary](http://dd.eionet.europa.eu/vocabulary/aq/pollutant/view) calls them "pollutant notation" and "pollutant id"

Discussion:
avaldebe: @JohnPaton 

It is OK io I merge this PR into the v1.0 branch?

Cheers,
Á.

PS:
No hurry, just pruning the issues and PRs
JohnPaton: Yes I think so. Let's put a deprecation warning in on master as well. Should probably have included that in 0.6, oh well 😄 we can finish the cleanup and then release an 0.7 that contains warnings.
avaldebe: Good point, I'll create a separate PR with the deprecation warnings targeting the master branch and remove them from this PR.

Patch:
diff --git a/README.md b/README.md
index 9e67180..998cf8a 100644
--- a/README.md
+++ b/README.md
@@ -34,23 +34,23 @@ $ pip install airbase
 ```pycon
 >>> import airbase
 >>> client = airbase.AirbaseClient()
->>> client.all_countries
-['GR', 'ES', 'IS', 'CY', 'NL', 'AT', 'LV', 'BE', 'CH', 'EE', 'FR', 'DE', ...
+>>> client.pollutants
+['k', 'CO', 'NO', 'O3', 'As', 'Cd', ...
 
->>> client.all_pollutants
-{'k': 412, 'CO': 10, 'NO': 38, 'O3': 7, 'As': 2018, 'Cd': 2014, ...
+>>> client.search_pollutant("O3")
+['O3', 'NO3', ...
 
->>> client.pollutants_per_country
-{'AD': [{'pl': 'CO', 'shortpl': 10}, {'pl': 'NO', 'shortpl': 38}, ...
+>>> client.countries
+['GR', 'ES', 'IS', 'CY', 'NL', 'AT', 'LV', 'BE', 'CH', 'EE', 'FR', 'DE', ...
 
->>> client.search_pollutant("O3")
-[{'pl': 'O3', 'shortpl': 7}, {'pl': 'NO3', 'shortpl': 46}, ...
+>>> client.pollutants_per_country
+{'AD': ['CO', 'NO', ...
 ```
 
 🗂 Request download links from the server and save the resulting CSVs into a directory:
 
 ```pycon
->>> r = client.request(country=["NL", "DE"], pl="NO3", year_from=2015)
+>>> r = client.request(country=["NL", "DE"], pollutant="NO3", year_from=2015)
 >>> r.download_to_directory(dir="data", skip_existing=True)
 Generating CSV download links...
 100%|██████████| 2/2 [00:03<00:00,  2.03s/it]
@@ -62,7 +62,7 @@ Downloading CSVs to data...
 💾 Or concatenate them into one big file:
 
 ```pycon
->>> r = client.request(country="FR", pl=["O3", "PM10"], year_to=2014)
+>>> r = client.request(country="FR", pollutant=["O3", "PM10"], year_to=2014)
 >>> r.download_to_file("data/raw.csv")
 Generating CSV download links...
 100%|██████████| 2/2 [00:12<00:00,  7.40s/it]
diff --git a/airbase/airbase.py b/airbase/airbase.py
index f0ff505..4f2e9e8 100644
--- a/airbase/airbase.py
+++ b/airbase/airbase.py
@@ -3,13 +3,9 @@ from __future__ import annotations
 import sys
 import warnings
 from datetime import datetime
+from itertools import product
 from pathlib import Path
 
-if sys.version_info >= (3, 8):
-    from typing import TypedDict
-else:
-    from typing_extensions import TypedDict
-
 from .fetch import (
     fetch_text,
     fetch_to_directory,
@@ -21,11 +17,6 @@ from .summary import DB
 from .util import link_list_url, string_safe_list
 
 
-class PollutantDict(TypedDict):
-    pl: str
-    shortpl: int
-
-
 class AirbaseClient:
     def __init__(self) -> None:
         """
@@ -51,38 +42,19 @@ class AirbaseClient:
         self._pollutants_ids = DB.pollutants()
 
         """The pollutants available in each country from AirBase."""
-        self.pollutants_per_country: dict[str, list[PollutantDict]] = dict()
-
-        for country, pollutants in DB.pollutants_per_country().items():
-            self.pollutants_per_country[country] = [
-                dict(pl=pl, shortpl=id) for pl, id in pollutants.items()
-            ]
+        self.pollutants_per_country = {
+            country: list(pollutants)
+            for country, pollutants in DB.pollutants_per_country().items()
+        }
 
     @property
-    def all_countries(self) -> list[str]:
-        warnings.warn(
-            f"{type(self).__qualname__}.all_countries has been deprecated and will be removed on v1. "
-            f"Use {type(self).__qualname__}.countries instead.",
-            DeprecationWarning,
-            stacklevel=2,
-        )
-        return self.countries
-
-    @property
-    def all_pollutants(self) -> dict[str, str]:
-        warnings.warn(
-            f"{type(self).__qualname__}.all_pollutants has been deprecated and will be removed on v1. "
-            f"Use {type(self).__qualname__}._pollutants_ids instead.",
-            DeprecationWarning,
-            stacklevel=2,
-        )
-        return self._pollutants_ids
+    def pollutants(self) -> list[str]:
+        return list(self._pollutants_ids)
 
     def request(
         self,
         country: str | list[str] | None = None,
-        pl: str | list[str] | None = None,
-        shortpl: str | list[str] | None = None,
+        pollutant: str | list[str] | None = None,
         year_from: str = "2013",
         year_to: str = CURRENT_YEAR,
         source: str = "All",
@@ -93,8 +65,7 @@ class AirbaseClient:
         """
         Initialize an AirbaseRequest for a query.
 
-        Pollutants can be specified either by name (`pl`) or by code
-        (`shortpl`). If no pollutants are specified, data for all
+        If no pollutants are specified, data for all
         available pollutants will be requested. If a pollutant is not
         available for a country, then we simply do not try to download
         those CSVs.
@@ -110,13 +81,8 @@ class AirbaseClient:
             country. Will raise ValueError if a country is not available
             on the server. If None, data for all countries will be
             requested. See `self.all_countries`.
-        :param pl: (optional) The pollutant(s) to request data
+        :param pollutant: (optional) The pollutant(s) to request data
             for. Must be one of the pollutants in `self.all_pollutants`.
-            Cannot be used in conjunction with `shortpl`.
-        :param shortpl: (optional). The pollutant code(s) to
-            request data for. Will be applied to each country requested.
-            Cannot be used in conjunction with `pl`.
-            Deprecated, will be removed on v1.
         :param year_from: (optional) The first year of data. Can
             not be earlier than 2013. Default 2013.
         :param year_to: (optional) The last year of data. Can not be
@@ -139,7 +105,7 @@ class AirbaseClient:
 
         :example:
             >>> client = AirbaseClient()
-            >>> r = client.request(["NL", "DE"], pl=["O3", "NO2"])
+            >>> r = client.request(["NL", "DE"], ["O3", "NO2"])
             >>> r.download_to_directory("data/raw")
             Generating CSV download links...
             100%|██████████| 4/4 [00:09<00:00,  2.64s/it]
@@ -156,22 +122,13 @@ class AirbaseClient:
             country = string_safe_list(country)
             self._validate_country(country)
 
-        if shortpl is not None:
-            warnings.warn(
-                f"the shortpl option has been deprecated and will be removed on v1. "
-                f"Use client.request([client._pollutants_ids[p] for p in shortpl], ...) instead.",
-                DeprecationWarning,
-                stacklevel=2,
-            )
-
-        if pl is not None and shortpl is not None:
-            raise ValueError("You cannot specify both 'pl' and 'shortpl'")
-
-        # construct shortpl form pl if applicable
-        if pl is not None:
-            pl_list = string_safe_list(pl)
+        if pollutant is None:
+            pollutant_id = None
+        else:
             try:
-                shortpl = [self._pollutants_ids[p] for p in pl_list]
+                pollutant_id = [
+                    self._pollutants_ids[p] for p in string_safe_list(pollutant)
+                ]
             except KeyError as e:
                 raise ValueError(
                     f"'{e.args[0]}' is not a valid pollutant name"
@@ -179,7 +136,7 @@ class AirbaseClient:
 
         return AirbaseRequest(
             country,
-            shortpl,
+            pollutant_id,
             year_from,
             year_to,
             source,
@@ -190,23 +147,22 @@ class AirbaseClient:
 
     def search_pollutant(
         self, query: str, limit: int | None = None
-    ) -> list[PollutantDict]:
+    ) -> list[str]:
         """
-        Search for a pollutant's `shortpl` number based on its name.
+        Search for a pollutant's ID number based on its name.
 
         :param query: The pollutant to search for.
         :param limit: (optional) Max number of results.
 
         :return: The best pollutant matches. Pollutants
-            are dicts with keys "pl" and "shortpl".
 
         :example:
             >>> AirbaseClient().search_pollutant("o3", limit=2)
-            >>> [{"pl": "O3", "shortpl": "7"}, {"pl": "NO3", "shortpl": "46"}]
+            >>> {"O3, "NO3"}
 
         """
         results = DB.search_pollutant(query, limit=limit)
-        return [dict(pl=pl, shortpl=id) for pl, id in results.items()]
+        return list(results)
 
     @staticmethod
     def download_metadata(filepath: str | Path, verbose: bool = True) -> None:
@@ -241,7 +197,7 @@ class AirbaseRequest:
     def __init__(
         self,
         country: str | list[str] | None = None,
-        shortpl: str | list[str] | None = None,
+        pollutant_id: str | list[str] | None = None,
         year_from: str = "2013",
         year_to: str = CURRENT_YEAR,
         source: str = "All",
@@ -260,7 +216,7 @@ class AirbaseRequest:
 
         :param country: 2-letter country code or a list of
             them. If a list, data will be requested for each country.
-        :param shortpl: (optional). The pollutant code to
+        :param pollutant_id: (optional). The pollutant code to
             request data for. Will be applied to each country requested.
             If None, all available pollutants will be requested. If a
             pollutant is not available for a country, then we simply
@@ -282,26 +238,17 @@ class AirbaseRequest:
             download links from the Airbase server at object
             initialization. Default False.
         """
-        self.country = country
-        self.shortpl = shortpl
-        self.year_from = year_from
-        self.year_to = year_to
-        self.source = source
-        self.update_date = update_date
-        self.verbose = verbose
 
-        self._country_list = string_safe_list(country)
-        self._shortpl_list = string_safe_list(shortpl)
-        self._download_links = []
+        self.verbose = verbose
 
-        for c in self._country_list:
-            for p in self._shortpl_list:
-                self._download_links.append(
-                    link_list_url(c, p, year_from, year_to, source, update_date)
-                )
+        countries = string_safe_list(country)
+        pollutant_ids = string_safe_list(pollutant_id)
+        self._download_links = [
+            link_list_url(c, p, year_from, year_to, source, update_date)
+            for c, p in product(countries, pollutant_ids)
+        ]
 
         self._csv_links: list[str] = []
-
         if preload_csv_links:
             self._get_csv_links()
 
diff --git a/airbase/cli.py b/airbase/cli.py
index 7437f29..7446fe1 100644
--- a/airbase/cli.py
+++ b/airbase/cli.py
@@ -21,7 +21,7 @@ class Country(str, Enum):
     _ignore_ = "country Country"  # type:ignore[misc]
 
     Country = vars()
-    for country in client.all_countries:
+    for country in client.countries:
         Country[country] = country
 
 
@@ -29,7 +29,7 @@ class Pollutant(str, Enum):
     _ignore_ = "poll Pollutant"  # type:ignore[misc]
 
     Pollutant = vars()
-    for poll in client.all_pollutants:
+    for poll in client.pollutants:
         Pollutant[poll] = poll
 
 
diff --git a/airbase/fetch.py b/airbase/fetch.py
index 70a47c3..bcb4ac9 100644
--- a/airbase/fetch.py
+++ b/airbase/fetch.py
@@ -56,7 +56,7 @@ def fetch_json(
 
     :param url: requested url
     :param timeout: maximum time to complete request (seconds)
-    :param encoding: text encoding used for decodding the response's body
+    :param encoding: text encoding used for decoding the response's body
 
     :return: decoded text from response's body as JSON
     """
@@ -100,12 +100,12 @@ async def fetcher(
     raise_for_status: bool = DEFAULT.raise_for_status,
     max_concurrent: int = DEFAULT.max_concurrent,
 ) -> AsyncIterator[str | Path]:
-    """Request multiple urls and write resquest text into individual paths
+    """Request multiple urls and write request text into individual paths
     it a `dict[url, path]` is provided, or return the decoded text from each request
     if only a `list[url]` is provided.
 
     :param urls: requested urls
-    :param encoding: text encoding used for decodding each response's body
+    :param encoding: text encoding used for decoding each response's body
     :param progress: show progress bar
     :param raise_for_status: Raise exceptions if download links
         return "bad" HTTP status codes. If False,
@@ -160,7 +160,7 @@ def fetch_unique_lines(
     """Request a list of url and return only the unique lines among all the responses
 
     :param urls: requested urls
-    :param encoding: text encoding used for decodding each response's body
+    :param encoding: text encoding used for decoding each response's body
     :param progress: show progress bar
     :param raise_for_status: Raise exceptions if download links
         return "bad" HTTP status codes. If False,
@@ -198,7 +198,7 @@ def fetch_to_file(
 
     :param urls: requested urls
     :param path: text file for all combined responses
-    :param encoding: text encoding used for decodding each response's body
+    :param encoding: text encoding used for decoding each response's body
     :param progress: show progress bar
     :param raise_for_status: Raise exceptions if download links
         return "bad" HTTP status codes. If False,
@@ -245,7 +245,7 @@ def fetch_to_directory(
     :param root: directory to write all responses
     :param bool skip_existing: Do not re-download url if the corresponding file
         is found in `root`
-    :param encoding: text encoding used for decodding each response's body
+    :param encoding: text encoding used for decoding each response's body
     :param progress: show progress bar
     :param raise_for_status: Raise exceptions if download links
         return "bad" HTTP status codes. If False,
diff --git a/airbase/resources.py b/airbase/resources.py
index 136126e..37a2a8e 100644
--- a/airbase/resources.py
+++ b/airbase/resources.py
@@ -8,7 +8,7 @@ LINK_LIST_URL_TEMPLATE = (
     "AirQualityDownload/AQData_Extract.fmw"
     "?CountryCode={country}"
     "&CityName="
-    "&Pollutant={shortpl}"
+    "&Pollutant={pollutant_id}"
     "&Year_from={year_from}"
     "&Year_to={year_to}"
     "&Station="
diff --git a/airbase/util.py b/airbase/util.py
index 0287596..61ca909 100644
--- a/airbase/util.py
+++ b/airbase/util.py
@@ -40,7 +40,7 @@ def string_safe_list(obj: str | Iterable[str] | None) -> list[str] | list[None]:
 
 def link_list_url(
     country: str | None,
-    shortpl: str | None = None,
+    pollutant_id: str | None = None,
     year_from: str = "2013",
     year_to: str = CURRENT_YEAR,
     source: str = "All",
@@ -51,7 +51,7 @@ def link_list_url(
 
     :param country: The 2-letter country code. See
         AirbaseClient.countries for options.
-    :param shortpl: (optional) The pollutant number. Leave blank to
+    :param pollutant_id: (optional) The pollutant number. Leave blank to
         get all pollutants. See AirbaseClient.pollutants_per_country for
         options.
     :param year_from: (optional) The first year of data. Can not be
@@ -87,7 +87,7 @@ def link_list_url(
 
     return LINK_LIST_URL_TEMPLATE.format(
         country=country or "",
-        shortpl=shortpl or "",
+        pollutant_id=pollutant_id or "",
         year_from=year_from,
         year_to=year_to,
         source=source,


Test patch:
diff --git a/tests/integration/test_airbase.py b/tests/integration/test_airbase.py
index 16059f8..adb61df 100644
--- a/tests/integration/test_airbase.py
+++ b/tests/integration/test_airbase.py
@@ -14,7 +14,7 @@ def client():
 
 def test_download_to_directory(client: airbase.AirbaseClient, tmp_path: Path):
     r = client.request(
-        country=["AD", "BE"], pl="CO", year_from="2017", year_to="2017"
+        country=["AD", "BE"], pollutant="CO", year_from="2017", year_to="2017"
     )
 
     r.download_to_directory(dir=str(tmp_path), skip_existing=True)
@@ -23,7 +23,7 @@ def test_download_to_directory(client: airbase.AirbaseClient, tmp_path: Path):
 
 def test_download_to_file(client: airbase.AirbaseClient, tmp_path: Path):
     r = client.request(
-        country="CY", pl=["As", "NO2"], year_from="2014", year_to="2014"
+        country="CY", pollutant=["As", "NO2"], year_from="2014", year_to="2014"
     )
 
     path = tmp_path / "raw.csv"
diff --git a/tests/test_airbase.py b/tests/test_airbase.py
index 25d6961..5a66db9 100644
--- a/tests/test_airbase.py
+++ b/tests/test_airbase.py
@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+from itertools import chain
 from pathlib import Path
 
 import pytest
@@ -10,7 +11,7 @@ from tests import resources
 
 @pytest.fixture
 def client(metadata_response) -> airbase.AirbaseClient:
-    """initialied client with mocked responses"""
+    """initialized client with mocked responses"""
     return airbase.AirbaseClient()
 
 
@@ -24,8 +25,22 @@ class TestAirbaseClient:
     def test_init(self):
         client = airbase.AirbaseClient()
         assert isinstance(client.countries, list)
-        assert isinstance(client._pollutants_ids, dict)
-        assert isinstance(client.pollutants_per_country, dict)
+
+        pollutants = client.pollutants
+        assert isinstance(pollutants, list)
+        assert all(isinstance(p, str) for p in pollutants)
+
+        pollutants_per_country = client.pollutants_per_country
+        assert isinstance(pollutants_per_country, dict)
+        assert all(isinstance(p, list) for p in pollutants_per_country.values())
+        assert all(
+            isinstance(p, str)
+            for p in chain.from_iterable(pollutants_per_country.values())
+        )
+
+        pollutants_ids = client._pollutants_ids
+        assert isinstance(pollutants_ids, dict)
+        assert list(pollutants_ids) == client.pollutants
 
     def test_download_metadata(
         self, tmp_path: Path, capsys, client: airbase.AirbaseClient
@@ -52,34 +67,29 @@ class TestAirbaseClient:
             client.request(year_to="9999")
 
     def test_request_pl(self, client: airbase.AirbaseClient):
-        r = client.request(pl="NO")
-        assert r.shortpl is not None
-        assert len(r.shortpl) == 1
+        r = client.request(pollutant="NO")
+        assert r._download_links
+        assert len(r._download_links) == len(client.countries)
 
-        r = client.request(pl=["NO", "NO3"])
-        assert r.shortpl is not None
-        assert len(r.shortpl) == 2
+        r = client.request(pollutant=["NO", "NO3"])
+        assert r._download_links
+        assert len(r._download_links) == 2 * len(client.countries)
 
         with pytest.raises(ValueError):
-            r = client.request(pl=["NO", "NO3", "Not a pl"])
+            r = client.request(pollutant=["NO", "NO3", "Not a pl"])
 
     def test_request_response_generated(self, client: airbase.AirbaseClient):
         r = client.request()
         assert isinstance(r, airbase.AirbaseRequest)
 
-    def test_request_not_pl_and_shortpl(self, client: airbase.AirbaseClient):
-        with pytest.raises(ValueError), pytest.warns(DeprecationWarning):
-            client.request(pl="O3", shortpl="123")
-
     def test_search_pl_exact(self, client: airbase.AirbaseClient):
         result = client.search_pollutant("NO3")
-        assert result[0]["pl"] == "NO3"
+        assert result[0] == "NO3"
 
     def test_search_pl_shortest_first(self, client: airbase.AirbaseClient):
         result = client.search_pollutant("N")
-        names: list[str] = [r["pl"] for r in result]
-        assert len(names[0]) <= len(names[1])
-        assert len(names[0]) <= len(names[-1])
+        assert len(result[0]) <= len(result[1])
+        assert len(result[0]) <= len(result[-1])
 
     def test_search_pl_limit(self, client: airbase.AirbaseClient):
         result = client.search_pollutant("N", limit=1)
@@ -91,7 +101,7 @@ class TestAirbaseClient:
 
     def test_search_pl_case_insensitive(self, client: airbase.AirbaseClient):
         result = client.search_pollutant("no3")
-        assert result[0]["pl"] == "NO3"
+        assert result[0] == "NO3"
 
 
 @pytest.mark.usefixtures("all_responses")