Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

g.region: Fix the flat flag #3216

Merged
merged 8 commits into from
Feb 9, 2024
Merged

g.region: Fix the flat flag #3216

merged 8 commits into from
Feb 9, 2024

Conversation

HuidaeCho
Copy link
Member

This PR prints projection= and zone= in the same one line for the flat -f flag.

Before:

$ g.region -pgf
projection=3
zone=0
n=37.0625 s=31.375 w=-109.0625 e=-103 nsres=0.03125 ewres=0.03125 rows=182 cols=194 cells=35308 

After:

$ g.region -pgf
projection=3 zone=0 n=37.0625 s=31.375 w=-109.0625 e=-103 nsres=0.03125 ewres=0.03125 rows=182 cols=194 cells=35308 

@HuidaeCho HuidaeCho added the bug Something isn't working label Oct 23, 2023
@HuidaeCho
Copy link
Member Author

HuidaeCho commented Oct 23, 2023

Maybe later for a new release in a separate PR, it would be better to imply -g when -f is used. Currently, both -g and -f are needed and -f without -g has no effect (surprise!). Just using -f should replace -gf IMO, UNLESS -f flattening is also used for other formats.

Or it just needs a better description because other flags are also accumulative as well?

@petrasovaa
Copy link
Contributor

petrasovaa commented Oct 24, 2023

There are print flags that add information (-l, -e, -c, -n, -3, -b). I don't think all of them are printed in flat format on 1 line with this PR yet.

I agree the -f flag should stand on its own in the same way you don't have to use -g flag together with -p flag. Ultimately we could introduce some kind of format method to include json format and get rid of all these flags?

There are some flags that don't seem to be behaving consistently, e.g. -t (no key printed), -w (always key=value format) and maybe some others.

@neteler neteler added this to the 8.4.0 milestone Nov 4, 2023
Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for making flat really one line. This strangeness was there for a while and it prevents use in shell scripting without addition of a grep filter.

A test for -gf and -f would be nice, e.g., testing that -f really yields one line only which was apparently missing. There is already general/g.region/testsuite/ so it should be relatively easy to add a new test.

@wenzeslaus
Copy link
Member

There are print flags that add information (-l, -e, -c, -n, -3, -b). I don't think all of them are printed in flat format on 1 line with this PR yet.

I think that falls into the general revision of all output formats below.

I agree the -f flag should stand on its own in the same way you don't have to use -g flag together with -p flag.

I also expect that. -g usually means "key=value" line by line in other tools and there are no modifications to the format.

Ultimately we could introduce some kind of format method to include json format and get rid of all these flags?

That would be ideal.

@github-actions github-actions bot added C Related code is in C module general labels Jan 23, 2024
@HuidaeCho
Copy link
Member Author

There are print flags that add information (-l, -e, -c, -n, -3, -b). I don't think all of them are printed in flat format on 1 line with this PR yet.

Done.

@HuidaeCho
Copy link
Member Author

There are print flags that add information (-l, -e, -c, -n, -3, -b). I don't think all of them are printed in flat format on 1 line with this PR yet.

I think that falls into the general revision of all output formats below.

I agree the -f flag should stand on its own in the same way you don't have to use -g flag together with -p flag.

I also expect that. -g usually means "key=value" line by line in other tools and there are no modifications to the format.

Ultimately we could introduce some kind of format method to include json format and get rid of all these flags?

That would be ideal.

Agreed, but I think that's for another PR.

@HuidaeCho
Copy link
Member Author

HuidaeCho commented Jan 23, 2024

There are some flags that don't seem to be behaving consistently, e.g. -t (no key printed), -w (always key=value format) and maybe some others.

Yep, GMT doesn't have a key. bbox= (already used with -w...) might be good? Or when -p prints the same info already, both -t and -w should be exclusive to -f? I think both -t and -w are meant to be standalone.

@github-actions github-actions bot added the Python Related code is in Python label Jan 31, 2024
Copy link
Contributor

@petrasovaa petrasovaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it and added a test for the f flag.

I tested the standalone -f flag, here is the code for it, but probably you are right this should be a separate PR.

diff --git a/general/g.region/main.c b/general/g.region/main.c
index 06c33a2e8c..33ad06f383 100644
--- a/general/g.region/main.c
+++ b/general/g.region/main.c
@@ -361,7 +361,7 @@ int main(int argc, char *argv[])
     G_option_required(
         flag.dflt, flag.savedefault, flag.print, flag.lprint, flag.eprint,
         flag.center, flag.gmt_style, flag.wms_style, flag.dist_res, flag.nangle,
-        flag.z, flag.bbox, flag.gprint, flag.res_set, flag.noupdate,
+        flag.z, flag.bbox, flag.gprint, flag.flprint, flag.res_set, flag.noupdate,
         parm.region, parm.raster, parm.raster3d, parm.vect, parm.north,
         parm.south, parm.east, parm.west, parm.top, parm.bottom, parm.rows,
         parm.cols, parm.res, parm.res3, parm.nsres, parm.ewres, parm.tbres,
@@ -371,7 +371,6 @@ int main(int argc, char *argv[])
                       flag.eprint, flag.center, flag.gmt_style, flag.wms_style,
                       flag.dist_res, flag.nangle, flag.z, flag.bbox,
                       flag.gprint, parm.save, NULL);
-    G_option_requires(flag.flprint, flag.gprint, NULL);
 
     if (G_parser(argc, argv))
         exit(EXIT_FAILURE);
@@ -383,7 +382,7 @@ int main(int argc, char *argv[])
     if (flag.print->answer)
         print_flag |= PRINT_REG;
 
-    if (flag.gprint->answer)
+    if (flag.gprint->answer || flat_flag)
         print_flag |= PRINT_SH;
 
     if (flag.lprint->answer)

Copy link
Member

@echoix echoix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read the changes and they make sense for what it says it does ;)

@echoix
Copy link
Member

echoix commented Feb 8, 2024

Created issues to not forget about the other points, so this PR should be good to merge, it was already reviewed and approved, and I took another look at it.

@HuidaeCho HuidaeCho merged commit 4ea78a7 into OSGeo:main Feb 9, 2024
25 checks passed
@HuidaeCho HuidaeCho deleted the g_region_flat branch February 9, 2024 14:46
jadenabrams100 pushed a commit to ncsu-csc472-spring2024/grass-CI-playground that referenced this pull request Feb 21, 2024
* g.region: Fix the flat flag

* Revert datum: and ellipsoid:

* Flatten other outputs

* Flat (-f) requires shell (-g) print

* Flatten WMS output

* add test

* fix formatting

---------

Co-authored-by: Anna Petrasova <kratochanna@gmail.com>
Co-authored-by: Edouard Choinière <27212526+echoix@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working C Related code is in C general module Python Related code is in Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants