GenericMappingTools / gmt

The Generic Mapping Tools
https://www.generic-mapping-tools.org
Other
861 stars 359 forks source link

Add longopt constants for near-common options -C, -G, -I and -W #7983

Closed PaulWessel closed 1 year ago

PaulWessel commented 1 year ago

Description of the desired feature

While the common GMT options have its own _gmt_connonlongopt.h file, there are many options that are near-common in that the usually have the exact same meaning in many modules, but were never selected to become common options (which always mean the same in any modules, e.g., -R, -J, -U, etc.).

At the moment we have one longopt constant for -I, i.e. --increment. It is defined as a constant in _gmtconstants.h thus:

#define GMT_INCREMENT_KW { '/', 'I', "increment|inc", "", "", "e,n", "exact,number" }

which allows any _longopt/*inc.h file that requires --increment to use that as here in _blockmeaninc.h:

...
{ 0, 'G', "outgrid",   "", "", "", "" },
GMT_INCREMENT_KW,   /* Defined in gmt_constant.h since not a true GMT common option (but almost) */
 { 0, 'S', "statistic",
...

@rbdavis, please add similar constants for -Ccpt [GMT_CPT_KW], -Goutgrid [GMT_OUTGRID_KW], and -Wpen [GMT_PEN_KW], and use these constants in module includes that use those options in the sense indicated. There are certainly modules that use -C for something else so we skip those of course. Also make sure to use GMT_INCREMENT_KW whenever grid increments are set. You will come across modules that need to set a pen but must but use some other option that -W since it may be taken for weights or other things. Not sure how we handle those since the 'W' is then hardwired into the constant. Any clever ideas to avoid duplicating GMT_PEN_KW to GMT_PENFp_KW (say for -Fppen in psmeca?

rbdavis commented 1 year ago

Will do.

rbdavis commented 1 year ago

I'll do this today, making the changes to gmt_constants.h only, and submit as PR for your review only. As soon as you approve I will s+m and then submit a follow-on PR, again for your review only, containing all related changes to existing longoptified modules. I would like to get this out of the way ASAP before submitting further PRs on new modules where these changes might be required.

Please advise if you'd like to proceed otherwise.

rbdavis commented 1 year ago

OK, I think this may be more complicated than expected, and believe we may have actually discussed this before now that I think of it.

Regarding -G, we have already longoptified multiple modules using -G to write an output grid, the problem is that many of their man pages differ in the -G details. For instance, the blockXXX modules allow no modifiers (or, more accurately, their man pages mention none), grdtrack specifies a +l (list) modifier only, and gmtbinstats and other modules have a whole host of modifiers +{d,n,o,s,c}, etc. What to do? You cannot just specify a simple entry like blockXXX's

{ 0, 'G', "outgrid", "", "", "", "" },

for all modules and then tack on modifiers at will (either short or long) like --outgrid+l or --outgrid+list -- . Once you trigger a longoption translation with --outgrid the only modifiers you can append are those explicitly declared in the translation definition.

I have not started looking yet but am concerned we may have the same issues with colormap and pen.

If you want to give me the exact and complete gmt_constants.h entries on any or all of the above and then suggest which modules they should be applied to I can do that, but I'm definitely worried this could be more of a timesink than a timesaver and may in fact break some modules whose current -G (etc.) behavior does not precisely conform to whatever quasi-global definition we come up with. I would not be surprised if some of the man pages are out-of-date with respect to any of these options, but unfortunately I have no way of knowing that and can only go by what I see there.

I guess I will put this task on hold for now and resume working on additional modules and tests until I get some additional guidance here, sorry!

rbdavis commented 1 year ago

I just had an idea for solving the -G confusion this morning while brushing my teeth a few minutes ago that I wanted to run by you. It's a kind of lazy pass-the-buck solution but I think it might work.

The translation table entries like the new ones you've asked me to create in gmt_constants.h are not intended as user-facing documentation which accurately describe usable features. Users should rely on manual pages, etc., for that knowledge. (This is the pass-the-buck aspect of this solution.) So, I don't see why we can't create a -G translation entry that translates all possible modifiers, etc., for all modules, and then give the user the responsibility for using only those modifiers which are supported for any particular module. The only thing we can't handle with this approach are conflicting modifiers, e.g., a +x modifier used by different modules where there are different intentions (i.e., requiring different long-option strings) in play. In fact, I suppose even that could be supported if we just used the alias translation feature.

I think this approach could work for essentially all of the -G module options I've seen to date. If we encounter any where it fails in the future then we just don't use the gmt_constants.h entry for that module.

What do you think? Or maybe this is what you were thinking to begin with and I just didn't pick up on it?

PaulWessel commented 1 year ago

Good idea, I think that should work. We can always catch inappropriate modifiers etc in the modules short-option parser. So allowing all to pass in and be converted to short format is good, since it is the module that will stomp on an unsupported modifier, not the parse. Pass-the-buck it is!

rbdavis commented 1 year ago

Will do, and will resume work on this issue after breakfast.

rbdavis commented 1 year ago

OK, things look reasonably good here with regard to this issue at least as far as modules which have already been longoptified. All of -G, -W and -C do have certain modifiers available in some modules but not others, but we should be able to handle that with the aforementioned buck-passing.

However, at least one module (pswiggle) has multiple options to set pen characteristics for different things and thus different longoption strings for each of these separate pen-like things (-T uses trackpen|track, -W uses outlinepen|pen). Also, pscoast uses --shorelines and not --pen. One thing to keep in mind is that whenever we enable an alias in either the common.h file or in a constants.h definition, that alias can then not be used for anything else in any individual *_inc.h files, so best not to go wild with aliases.

I think I'm ready to go on a PR for just the changes to constants.h, which are now:

#define GMT_INCREMENT_KW { '/', 'I', "increment|inc", "", "", "e,n", "exact,number" }
#define GMT_CPT_KW      { '/', 'C', "cpt|cmap", "", "", "h,i,u,U", "hinge,zinc,fromunit,tounit" }
#define GMT_OUTGRID_KW  { '/', 'G', "outgrid", "", "", "d,n,o,s,c,l", "divide,nan,offset,scale,gdal,list" }
#define GMT_PEN_KW      { '/', 'W', pen", "", "", "c", "color" }

I recommend that we get this approved (assuming you are OK with the above) and merged ASAP, then I can proceed to changing existing longoptified *_inc.h files to use those definitions, and we can discuss along the way which modules should or should not do so (like the -W-related ones above).

PaulWessel commented 1 year ago

Sure, do a PR and I will check it tomorrow (sorry bedtime as I have early blood test at the hospital tomorrow - and it has snowed a foot...

rbdavis commented 1 year ago

Will do. Doing some testing now to ensure no syntax errs, then will submit. Good luck at the hospital tomorrow!