search

ubarsc / python-fmask

A set of command line utilities and Python modules that implement the ‘fmask’ algorithm
https://www.pythonfmask.org
GNU General Public License v3.0
74 stars 21 forks source link

python script support #41

Closed floriandeboissieu closed 4 years ago

floriandeboissieu commented 4 years ago

Current version is made for command line only, i.e. command line arguments are taken from sys.argv. I propose to add an argv argument to mainRoutine so that it can be called directly from a python session with a list of arguments. Not as clean as a list of named arguments, but an easy way to extend support to python scripting without moodifying to much your code. Example:

from fmask.cmdline import sentinel2Stacked
sentinel2Stacked.mainRoutine(['--safedir', '<directory>', '-o', '<output directory>', '--parallaxtest'])

I have done it with sentinel2Stacked, but it could be easily extended to others mainRoutines.

neilflood commented 4 years ago

I agree with @gillins, this doesn't seem to actually do anything.

neilflood commented 4 years ago

Could you expand the docstrings for mainRoutine and getCmdargs, to explain what argv should contain, and when it should be given? Something like "If argv is given, it should be......", which will explain under what circumstances it is or is not required.

floriandeboissieu commented 4 years ago

Sure, is that ok?

neilflood commented 4 years ago

Sorry about the delay, a few other things going on.

Two points:

First, the getCmdargs() function has no capital A. Best to have that right in the docstring.

Second, could you assure me that you have actually tested this precise version of the code? It looks like you have constructed this pull request purely within github, and so it does not appear in your fork of the repository. I just cloned your fork, and it is not there. This means that I can't test it to check for any problems. The code looks fine to me on the screen, but I know I am not clever enough to always see problems, so I prefer to do a live test of the change.

I don't know if it is possible to apply the change back to your code, or whether you will need to start again. However, I need to be able to see that this exact version of the changes has been tested at least once before I will merge it.

Sorry, but I have enough experience to know that code which looks right is not always right.

neilflood commented 4 years ago

Sorry - cancel that last comment about the code not being in your fork. Too early in the morning, and I had forgotten to switch branches. My apologies.

I will test now.

neilflood commented 4 years ago

OK, test seems fine. Fix that capital A, and I will merge.