Repository: eyeseast/geocode-sqlite (language: python)

Issue:
Better argument ergonomics
Looking at this with fresh eyes, this whole bit is dumb:

> Note the order of options: There are two sets of options we need to pass.
> 
> The first concerns the data we're geocoding. We need to say where our database is and what table we're using, and optionally, how to extract a location query.
> 
> Then, we need to say what geocoder we're using, and pass in any options needed to initalize it. This will be different for each geocoder we want to use.

Ideally, it should be something like this:

```sh
geocode-sqlite nominatum  data.db data \
 --location="{address}, {city}, {state} {zip}" \
 --delay=1 \
 --user-agent="this-is-me"
```

*Maybe* this would be ok:

```sh
geocode-sqlite
 --location="{address}, {city}, {state} {zip}" \
 --delay=1 \
 --user-agent="this-is-me" \
nominatum data.db data
```

But the first way is definitely better.



Discussion / hints:
This would probably do a lot to help #2, since doing `geocode-sqlite nominatum` alone would trigger a help callback.

Fix patch:
diff --git a/.gitignore b/.gitignore
index 56108ac..9144fcb 100644
--- a/.gitignore
+++ b/.gitignore
@@ -16,4 +16,5 @@ dist/
 *.db-shm
 *.sqlite
 
-.env
\ No newline at end of file
+.env
+.vscode
\ No newline at end of file
diff --git a/Makefile b/Makefile
index ba0c736..a905978 100644
--- a/Makefile
+++ b/Makefile
@@ -6,38 +6,33 @@ tests/%.db: tests/innout.geojson tests/innout.csv
 
 .PHONY: test
 test: tests/test.db
-	geocode-sqlite -l "{id}" -d 0 $^ innout_test test -p $^
+	geocode-sqlite test tests/test.db innout_test -p tests/test.db -l "{id}" -d 0
 
 .PHONY: nominatum
 nominatum: tests/nominatum.db
-	geocode-sqlite -l "{full}, {city}, {state} {postcode}" \
-	 --delay 1 \
-	 $^ \
-	 innout_test \
-	 nominatum \
-	 --user-agent "geocode-sqlite"
+	geocode-sqlite nominatum $^ innout_test \
+		--location "{full}, {city}, {state} {postcode}" \
+		--delay 1 \
+		--user-agent "geocode-sqlite"
 
 .PHONY: mapquest
 mapquest: tests/mapquest.db
-	geocode-sqlite -l "{full}, {city}, {state} {postcode}" \
-	$^ innout_test \
-	open-mapquest \
-	--api-key "$(MAPQUEST_API_KEY)"
+	geocode-sqlite open-mapquest $^ innout_test \
+		--location "{full}, {city}, {state} {postcode}" \
+		--api-key "$(MAPQUEST_API_KEY)"
 
 .PHONY: google
 google: tests/google.db
-	geocode-sqlite -l "{full}, {city}, {state} {postcode}" \
-	$^ innout_test \
-	googlev3 \
-	--api-key "$(GOOGLE_API_KEY)"
-
+	geocode-sqlite googlev3 $^ innout_test \
+		--location "{full}, {city}, {state} {postcode}" \
+		--api-key "$(GOOGLE_API_KEY)"
 
 .PHONY: bing
 bing: tests/bing.db
-	geocode-sqlite -l "{full}, {city}, {state} {postcode}" -d 1 \
-	$^ innout_test \
-	bing \
-	--api-key "$(BING_API_KEY)"
+	geocode-sqlite bing $^ innout_test \
+		--location "{full}, {city}, {state} {postcode}" \
+		--delay 1 \
+		--api-key "$(BING_API_KEY)"
 
 
 .PHONY: clean
diff --git a/README.md b/README.md
index 02cba27..5bc0a86 100644
--- a/README.md
+++ b/README.md
@@ -31,31 +31,23 @@ sqlite-utils insert data.db data data.csv --csv
 Now, geocode it using OpenStreetMap's Nominatum geocoder.
 
 ```sh
-geocode-sqlite
+geocode-sqlite nominatum data.db data \
  --location="{address}, {city}, {state} {zip}" \
  --delay=1 \
- data.db data \
- nominatum  \
  --user-agent="this-is-me"
 ```
 
-In the command above, you're using Nominatum, which is free and only asks for a unique user agent.
+In the command above, you're using Nominatum, which is free and only asks for a unique user agent (`--user-agent`).
 
 This will connect to a database (`data.db`) and read all rows from the table `data` (skipping any that already
 have both a `latitude` and `longitude` column filled).
 
-You're also telling the geocoder how to extract a location query from a row of data, using Python's
-built-in string formatting, and setting a rate limit of one request per second.
+You're also telling the geocoder how to extract a location query (`--location`) from a row of data, using Python's
+built-in string formatting, and setting a rate limit (`--delay`) of one request per second.
 
 For each row where geocoding succeeds, `latitude` and `longitude` will be populated. If you hit an error, or a rate limit,
 run the same query and pick up where you left off.
 
-**Note the order of options**: There are two sets of options we need to pass.
-
-The first concerns the data we're geocoding. We need to say where our database is and what table we're using, and optionally, how to extract a location query.
-
-_Then_, we need to say what geocoder we're using, and pass in any options needed to initalize it. This will be different for each geocoder we want to use.
-
 Under the hood, this package uses the excellent [geopy](https://geopy.readthedocs.io/en/latest/) library, which is stable and thoroughly road-tested. If you need help understanding a particular geocoder's options, consult [geopy's documentation](https://geopy.readthedocs.io/en/latest/#module-geopy.geocoders).
 
 ### Supported Geocoders
@@ -69,6 +61,22 @@ The CLI currently supports these geocoders:
 
 More will be added soon.
 
+### Common arguments and options
+
+Each geocoder needs to know where to find the data it's working with. These are the first two arguments:
+
+- database: a path to a SQLite file, which must already exist
+- table: the name of a table, in that database, which exists and has data to geocode
+
+From there, we have a set of options passed to every geocoder:
+
+- location: a [string format](https://docs.python.org/3/library/stdtypes.html#str.format) that will be expanded with each row to build a full query, to be geocoded
+- delay: a delay between each call (some services require this)
+- latitude: latitude column name
+- longitude: longitude column name
+
+Each geocoder takes additional, specific arguments beyond these, such as API keys. Again, [geopy's documentation](https://geopy.readthedocs.io/en/latest/#module-geopy.geocoders) is an excellent resource.
+
 ## Python API
 
 The command line interface aims to support the most common options for each geocoder. For more find-grained control, use the Python API.
diff --git a/geocode_sqlite/cli.py b/geocode_sqlite/cli.py
index 33c8847..66fb474 100644
--- a/geocode_sqlite/cli.py
+++ b/geocode_sqlite/cli.py
@@ -7,42 +7,65 @@
 from .testing import DummyGeocoder
 
 
-def validate_database(ctx, param, value):
-    """
-    Handle cases where a user calls something like:
-    geocode-sqlite bing --help
-    """
-    subs = set(cli.list_commands(ctx))
-    if value in subs:
-        cmd = cli.get_command(ctx, value)
-        ctx.info_name = f"geocode-sqlite ... {cmd.name}"
-        click.echo(cmd.get_help(ctx))
-        return ctx.exit()
+def common_options(f):
+    f = click.pass_context(f)
+
+    # arguments have to be added in reverse order
+    f = click.argument("table", type=click.STRING, required=True)(f)
+    f = click.argument(
+        "database",
+        type=click.Path(exists=True, file_okay=True, dir_okay=False, allow_dash=False),
+        required=True,
+    )(f)
+
+    # options
+    f = click.option("-l", "--location", type=click.STRING, default="{location}")(f)
+    f = click.option("-d", "--delay", type=click.FLOAT, default=1.0)(f)
+    f = click.option("--latitude", type=click.STRING, default="latitude")(f)
+    f = click.option("--longitude", type=click.STRING, default="longitude")(f)
+
+    return f
+
+
+def fill_context(ctx, database, table, location, delay, latitude, longitude):
+    "Add common options to context"
+    ctx.obj.update(
+        database=database,
+        table=table,
+        location=location,
+        delay=delay,
+        latitude=latitude,
+        longitude=longitude,
+    )
 
-    return value
+
+def extract_context(ctx):
+    "The opposite of fill_context. Return all common args in order."
+    return (
+        ctx.obj["database"],
+        ctx.obj["table"],
+        ctx.obj["location"],
+        ctx.obj["delay"],
+        ctx.obj["latitude"],
+        ctx.obj["longitude"],
+    )
 
 
 @click.group()
 @click.version_option()
-@click.argument(
-    "database",
-    type=click.Path(exists=False, file_okay=True, dir_okay=False, allow_dash=False),
-    required=True,
-    callback=validate_database,
-)
-@click.argument("table", type=click.STRING, required=True)
-@click.option("-l", "--location", type=click.STRING, default="{location}")
-@click.option("-d", "--delay", type=click.FLOAT, default=1.0)
-@click.option("--latitude", type=click.STRING, default="latitude")
-@click.option("--longitude", type=click.STRING, default="longitude")
-def cli(database, table, location, delay, latitude, longitude):
+@click.pass_context
+def cli(ctx):
     "Geocode rows from a SQLite table"
+    ctx.ensure_object(dict)
 
 
 @cli.resultcallback()
-def geocode(geocoder, database, table, location, delay, latitude, longitude):
+@click.pass_context
+def geocode(ctx, geocoder):
     "Do the actual geocoding"
+    database, table, location, delay, latitude, longitude = extract_context(ctx)
     click.echo(f"Geocoding table: {table}")
+
     if latitude != "latitude":
         click.echo(f"Using custom latitude field: {latitude}")
 
@@ -62,59 +85,72 @@ def geocode(geocoder, database, table, location, delay, latitude, longitude):
 
 
 @cli.command("test", hidden=True)
+@common_options
 @click.option("-p", "--db-path", type=click.Path(exists=True))
-def use_tester(db_path):
+def use_tester(ctx, database, table, location, delay, latitude, longitude, db_path):
     "Only use this for testing"
     click.echo(f"Using test geocoder with database {db_path}")
+    fill_context(ctx, database, table, location, delay, latitude, longitude)
     return DummyGeocoder(Database(db_path))
 
 
 @cli.command("bing")
+@common_options
 @click.option(
     "-k", "--api-key", type=click.STRING, required=True, envvar="BING_API_KEY"
 )
-def bing(api_key):
+def bing(ctx, database, table, location, delay, latitude, longitude, api_key):
     "Bing"
     click.echo("Using Bing geocoder")
+    fill_context(ctx, database, table, location, delay, latitude, longitude)
     return geocoders.Bing(api_key=api_key)
 
 
 @cli.command("googlev3")
+@common_options
 @click.option(
     "-k", "--api-key", type=click.STRING, required=True, envvar="GOOGLE_API_KEY"
 )
 @click.option("--domain", type=click.STRING, default="maps.googleapis.com")
-def google(api_key, domain):
+def google(ctx, database, table, location, delay, latitude, longitude, api_key, domain):
     "Google V3"
     click.echo(f"Using GoogleV3 geocoder at domain {domain}")
+    fill_context(ctx, database, table, location, delay, latitude, longitude)
     return geocoders.GoogleV3(api_key=api_key, domain=domain)
 
 
 @cli.command("mapquest")
+@common_options
 @click.option(
     "-k", "--api-key", type=click.STRING, required=True, envvar="MAPQUEST_API_KEY"
 )
-def mapquest(api_key):
+def mapquest(ctx, database, table, location, delay, latitude, longitude, api_key):
     "Mapquest"
     click.echo("Using MapQuest geocoder")
+    fill_context(ctx, database, table, location, delay, latitude, longitude)
     return geocoders.MapQuest(api_key=api_key)
 
 
 @cli.command()
+@common_options
 @click.option("--user-agent", type=click.STRING)
 @click.option("--domain", type=click.STRING, default="nominatim.openstreetmap.org")
-def nominatum(user_agent, domain):
+def nominatum(
+    ctx, database, table, location, delay, latitude, longitude, user_agent, domain
+):
     "Nominatum (OSM)"
     click.echo(f"Using Nominatum geocoder at {domain}")
+    fill_context(ctx, database, table, location, delay, latitude, longitude)
     return geocoders.Nominatim(user_agent=user_agent, domain=domain)
 
 
 @cli.command("open-mapquest")
+@common_options
 @click.option(
     "-k", "--api-key", type=click.STRING, required=True, envvar="MAPQUEST_API_KEY"
 )
-def open_mapquest(api_key):
+def open_mapquest(ctx, database, table, location, delay, latitude, longitude, api_key):
     "Open Mapquest"
     click.echo("Using MapQuest geocoder")
+    fill_context(ctx, database, table, location, delay, latitude, longitude)
     return geocoders.MapQuest(api_key=api_key)
-


Test patch:
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index f65656d..8008f25 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -7,24 +7,23 @@ jobs:
     runs-on: ubuntu-latest
     strategy:
       matrix:
-        python-version: [3.6, 3.7, 3.8]
+        python-version: [3.6, 3.7, 3.8, 3.9]
     steps:
-    - uses: actions/checkout@v2
-    - name: Set up Python ${{ matrix.python-version }}
-      uses: actions/setup-python@v2
-      with:
-        python-version: ${{ matrix.python-version }}
-    - uses: actions/cache@v2
-      name: Configure pip caching
-      with:
-        path: ~/.cache/pip
-        key: ${{ runner.os }}-pip-${{ hashFiles('**/setup.py') }}
-        restore-keys: |
-          ${{ runner.os }}-pip-
-    - name: Install dependencies
-      run: |
-        pip install -e '.[test]'
-    - name: Run tests
-      run: |
-        pytest
-
+      - uses: actions/checkout@v2
+      - name: Set up Python ${{ matrix.python-version }}
+        uses: actions/setup-python@v2
+        with:
+          python-version: ${{ matrix.python-version }}
+      - uses: actions/cache@v2
+        name: Configure pip caching
+        with:
+          path: ~/.cache/pip
+          key: ${{ runner.os }}-pip-${{ hashFiles('**/setup.py') }}
+          restore-keys: |
+            ${{ runner.os }}-pip-
+      - name: Install dependencies
+        run: |
+          pip install -e '.[test]'
+      - name: Run tests
+        run: |
+          pytest
diff --git a/tests/test_geocode_sqlite.py b/tests/test_geocode_sqlite.py
index 3447a33..a07cc69 100644
--- a/tests/test_geocode_sqlite.py
+++ b/tests/test_geocode_sqlite.py
@@ -64,15 +64,15 @@ def test_cli_geocode_table(db, geocoder):
     result = runner.invoke(
         cli,
         [
-            "-l",
+            "test",  # geocoder subcommand
+            str(DB_PATH),  # db
+            str(TABLE_NAME),  # table
+            "--db-path",  # path, for test geocoder
+            str(DB_PATH),
+            "--location",  # location
             "{id}",
-            "-d",
+            "--delay",  # delay
             "0",
-            str(DB_PATH),
-            str(TABLE_NAME),
-            "test",
-            "-p",
-            str(DB_PATH),
         ],
     )
 
@@ -94,7 +94,12 @@ def test_custom_fieldnames(db, geocoder):
     result = runner.invoke(
         cli,
         [
-            "--location",
+            "test",
+            str(DB_PATH),
+            str(TABLE_NAME),
+            "-p",
+            str(DB_PATH),
+            "-l",
             "{id}",
             "-d",
             "0",
@@ -102,11 +107,6 @@ def test_custom_fieldnames(db, geocoder):
             "lat",
             "--longitude",
             "lng",
-            str(DB_PATH),
-            str(TABLE_NAME),
-            "test",
-            "-p",
-            str(DB_PATH),
         ],
     )
 
@@ -139,15 +139,15 @@ def test_rate_limiting(db, geocoder):
     result = runner.invoke(
         cli,
         [
-            "--location",
-            "{id}",
-            "--delay",
-            "1",
+            "test",
             str(DB_PATH),
             str(TABLE_NAME),
-            "test",
             "-p",
             str(DB_PATH),
+            "--location",
+            "{id}",
+            "--delay",
+            "1",
         ],
     )
     end = datetime.datetime.now()