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

Patch 5561 - Standardized return codes for gdal-utils #5582

Merged
merged 33 commits into from
May 27, 2022

Conversation

maphew
Copy link
Contributor

@maphew maphew commented Apr 6, 2022

What does this PR do?

Standardized return codes for gdal-utils scripts when called without arguments

What are related issues/pull requests?

Tasklist

  • Change return codes to 2 that were
    • -1
    • 0 auxillary Skip, these are not meant to be scripts.
    • - 1
    • 2 scripts that use GDALArgumentParser (ref)
  • Add test case(s)
  • Add documentation
  • Review
  • Adjust for comments
  • All CI builds and checks have passed

Environment

Provide environment details, if relevant:

  • Python 3

Updated 2022-05-23

@maphew maphew marked this pull request as draft April 6, 2022 06:11
@maphew maphew changed the title Patch 5561 - Standardized return codes for **gdal-utils** scripts - DRAFT Patch 5561 - Standardized return codes for gdal-utils - DRAFT Apr 7, 2022
@rcoup
Copy link
Member

rcoup commented May 11, 2022

Is there a reason we can't standardise on 2 everywhere?

ie:
1 = general error
2 = you provided some bad arguments / no arguments — this should have been expected
other numbers = specific error

@maphew
Copy link
Contributor Author

maphew commented May 11, 2022

Define 'everywhere' :) If you're talking about gdal_translate and friends, yes I think that's a good idea, but is beyond the scope of this PR (and more importantly my skills!) It is my motivation to standardize return codes and other related things in the python scripts portion of gdal.

Key was understanding that the script name (with path) need to be in
subprocess.run() initial [...] block, and _not_ as part of input arguments.

More refactoring needed now to demark the difference between Programs
and Scripts, with explanation. Probably some simplification possible to.
@maphew maphew changed the title Patch 5561 - Standardized return codes for gdal-utils - DRAFT Patch 5561 - Standardized return codes for gdal-utils May 24, 2022
@maphew
Copy link
Contributor Author

maphew commented May 24, 2022

Wow, what a long haul this 'simple' idea turned into! But, I think it's finally ready. Please have a look @rouault, @idanmiara.

fft.py fails flake8 test because of import *. I'm unfamiliar with fourier transform module(s) and don't know what the correct remedy is here.

@rouault
Copy link
Member

rouault commented May 26, 2022

fft.py fails flake8 test because of import *

I don't understand what you mean. I only see a "import FFT" in that file

maphew added a commit to maphew/gdal-plus that referenced this pull request May 27, 2022
Run a series of python scripts and display their exit codes. Helper script for: Standardize gdal-utils scripts return codes #5561
(OSGeo/gdal#5582)
Not actually gdal specific, hard coded for now but relatively easy to generalize for any folder.
Probably better to use numpy.fft instead, but it doesn't have function
'fft2d' and I don't know what the best equivalent function is.
@maphew
Copy link
Contributor Author

maphew commented May 27, 2022

fft.py fails flake8 test because of import *

I don't understand what you mean. I only see a "import FFT" in that file

try:
    import FFT
except ModuleNotFoundError:
    from numpy.fft import *

Oh! Git blame says I'm the one who added it, in 04aad75, to work around an import error. I don't remember what made me think that was a good alternative. I've changed it to gracefully exit if FFT module not found. It's might be better to use numpy.fft instead, but it doesn't have function 'fft2d' and I don't know what the best equivalent function is.

@maphew maphew marked this pull request as ready for review May 27, 2022 04:37
@rouault rouault added this to the 3.6.0 milestone May 27, 2022
@rouault rouault merged commit cec9989 into OSGeo:master May 27, 2022
@maphew maphew deleted the patch-5561 branch May 27, 2022 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants