Argument parsing for h3ToGeo, h3ToGeoBoundary, geoToH3 #227

isaacbrodsky · 2019-05-01T18:43:46Z

Continues from #224 to rewrite argument parsing for these three filter applications.

coveralls · 2019-05-01T18:50:16Z

Coverage remained the same at 100.0% when pulling 7f3de95 on isaacbrodsky:args-h3-to-geo into a6cd31b on uber:master.

nrabinowitz · 2019-05-02T18:53:13Z

src/apps/applib/lib/utility.c

@@ -68,12 +68,12 @@ int parseArgs(int argc, char* argv[], int numArgs, Arg* args[],
    const char* errorMessage = NULL;
    const char* errorDetails = NULL;

-    int failed = _parseArgsList(argc, argv, 4, args, helpArg, &errorMessage,


This was a bug from the last diff?

Yes, this was a copy paste error when refactoring the code. I found it from a segfault when testing another filter with only three arguments.

nrabinowitz · 2019-05-02T18:58:17Z

src/apps/filters/geoToH3.c

+                  .valueName = "res",
+                  .value = &res,
+                  .helpText = "Resolution, 0-15 inclusive."};
+    Arg latArg = {.names = {"-lat", "--latitude"},


Nit: I'd have thought --lat and --lon would be the short forms - I've generally seen single-dash only for single-char args (partly so that if we wanted we could stack like -xvf).

One one-char option would be to use -y for lat and -x for long.

Do you have a preference between adding --lat/--lon or -y/-x? I am leaning towards --lat/--lon since it does not have the ambiguity about which comes first.

To me -y / -x sounds like either planar coordinates or two components of 3D ECEF coordinates. Both might be potential arguments to some future program (especially given that H3 can now output planar IJK grids).

nrabinowitz

LG overall, most comments on clearing up the --kml-name params.

nrabinowitz · 2019-05-02T19:00:03Z

src/apps/filters/geoToH3.c

+                error("parsing lat/lon");
+
+            // convert to H3
+            GeoCoord g;


Nit: Worth moving these 4 lines into a separate function to DRY out the two branches?

src/apps/filters/h3ToGeo.c

nrabinowitz · 2019-05-02T23:16:57Z

src/apps/filters/h3ToGeo.c

+                      .scanFormat = "%255c",  // BUFF_SIZE - 1
+                      .valueName = "name",
+                      .value = &userKmlName,
+                      .helpText = "Name of the KML file."};


This isn't correct, right? Isn't this name used inside a tag in KML, rather than the file name?

I had some difficulty with this because the tag is named "name" rather than "title", which I felt would be clearly not the name of the file. I think rewriting it to mention the tag and not refer to a "file" is a good idea.

nrabinowitz · 2019-05-02T23:17:25Z

src/apps/filters/h3ToGeo.c

 *
 *  Examples:
 *
 *     `h3ToGeo < indexes.txt`
 *        - outputs plain text cell center points for the H3 indexes contained
 *          in the file `indexes.txt`
 *
- *     `h3ToGeo 1 "kml file" "h3 cells" < indexes.txt > cells.kml`
+ *     `h3ToGeo --kml --kml-name "kml file" --kml-description "h3 cells" <


As below, should this read "kml file" or "My KML Title"

src/apps/filters/h3ToGeo.c

nrabinowitz · 2019-05-02T23:26:12Z

src/apps/filters/h3ToGeoBoundary.c

 *
 *  Examples:
 *
 *     `h3ToGeoBoundary < indexes.txt`
 *        - outputs plain text cell boundaries for the H3 indexes contained
 *          in the file `indexes.txt`
 *
- *     `h3ToGeoBoundary 1 "kml file" "h3 cells" < indexes.txt > cells.kml`
+ *     `h3ToGeoBoundary --kml --kml-name "kml file" --kml-description "h3 cells"


Ditto on "kml file"

src/apps/filters/h3ToGeoBoundary.c

nrabinowitz · 2019-05-02T23:28:42Z

src/apps/filters/kRing.c

    Arg kArg = {.names = {"-k", NULL},
                .required = true,
                .scanFormat = "%d",
                .valueName = "k",
                .value = &k,
-                .helpText = "Radius of hexagons"};
+                .helpText = "Radius of hexagons."};


Nit: Might be clearer as "Radius of ring in hexagons"

I'm not sure I understand that since this would produce the neighborhood around the index (cf #42) rather than a hollow ring.

Per offline: Radius in hexagons.

src/apps/filters/kRing.c

nrabinowitz · 2019-05-16T23:24:30Z

src/apps/filters/h3ToGeo.c

+            "Index, or not specified to read indexes from standard in."};
+    Arg kmlArg = {.names = {"-k", "--kml"},
+                  .helpText = "Print output in KML format."};
+    Arg kmlNameArg = {.names = {"--kn", "--kml-name"},


Here I might have gone for -n and -d, but that's just bikeshedding.

Argument parsing for h3ToGeo, h3ToGeoBoundary, geoToH3

Argument parsing for h3ToGeo, h3ToGeoBoundayr, geoToH3

2b5bd7b

isaacbrodsky requested review from nrabinowitz, dfellis and jogly May 1, 2019 18:43

Isaac Brodsky added 2 commits May 1, 2019 13:49

Print argument validation error with the help message.

aaf79ae

Pass the actual number of arguments

0c28636

nrabinowitz reviewed May 2, 2019

View reviewed changes

Abbreviate arguments with -- when not single character

1c5cb75

isaacbrodsky force-pushed the args-h3-to-geo branch from 7531329 to 1c5cb75 Compare May 2, 2019 20:00

nrabinowitz reviewed May 2, 2019

View reviewed changes

KML help text

1f53dc7

nrabinowitz changed the title ~~Argument parsing for h3ToGeo, h3ToGeoBoundayr, geoToH3~~ Argument parsing for h3ToGeo, h3ToGeoBoundary, geoToH3 May 4, 2019

nrabinowitz reviewed May 16, 2019

View reviewed changes

nrabinowitz approved these changes May 16, 2019

View reviewed changes

Isaac Brodsky added 2 commits May 16, 2019 16:41

Adjust arg description

f1c2199

Merge branch 'master' into args-h3-to-geo

7f3de95

isaacbrodsky merged commit fc8c80a into uber:master May 17, 2019

isaacbrodsky deleted the args-h3-to-geo branch May 17, 2019 17:37

mrdvt92 pushed a commit to mrdvt92/h3 that referenced this pull request Jun 19, 2022

Merge pull request uber#227 from isaacbrodsky/args-h3-to-geo

6b66f70

Argument parsing for h3ToGeo, h3ToGeoBoundary, geoToH3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Argument parsing for h3ToGeo, h3ToGeoBoundary, geoToH3 #227

Argument parsing for h3ToGeo, h3ToGeoBoundary, geoToH3 #227

isaacbrodsky commented May 1, 2019

coveralls commented May 1, 2019 •

edited

Loading

nrabinowitz May 2, 2019

isaacbrodsky May 2, 2019 •

edited

Loading

nrabinowitz May 2, 2019

isaacbrodsky May 2, 2019

sahrk May 3, 2019

nrabinowitz left a comment

nrabinowitz May 2, 2019

nrabinowitz May 2, 2019

isaacbrodsky May 2, 2019

nrabinowitz May 2, 2019

nrabinowitz May 2, 2019

nrabinowitz May 2, 2019

isaacbrodsky May 2, 2019

nrabinowitz May 16, 2019

nrabinowitz May 16, 2019

Argument parsing for h3ToGeo, h3ToGeoBoundary, geoToH3 #227

Argument parsing for h3ToGeo, h3ToGeoBoundary, geoToH3 #227

Conversation

isaacbrodsky commented May 1, 2019

coveralls commented May 1, 2019 • edited Loading

Choose a reason for hiding this comment

isaacbrodsky May 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrabinowitz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 1, 2019 •

edited

Loading

isaacbrodsky May 2, 2019 •

edited

Loading