Return values

[pekkaklarck]

I'm using pss in a shell script and want to do different actions depending are there matches or not. I can do that based on does pss write anything to stdout:

if [[ -n "$(pss pattern location)" ]]; then 
     echo match
fi

It would be a bit more convenient to do this based on the return value:

if pss patten location > /dev/null; then
    echo match
fi

Because pss returns 0 regardless does it match anything or not, the above always echos "match". I don't know about ack, but at least grep returns 0 if there is a match, 1 if there is no match, and 2 if there is an error. I would be convenient if pss followed these rules.

[eliben]

Behaving like grep here makes sense to me. Feel free to send a pull request.

[pekkaklarck]

Cool, I'll take a look at this. Been accepting pull requests from others in our project lately and it's fun to be on this side.

Would you be fine if I change the logic so that main() doesn't use sys.exit() but instead returns the rc and pss(.py) script then uses sys.exit(main())? main() exiting isn't, in my opinion, the best practice, but obviously this would be somewhat backwards incompatible change. If you are OK with this change, should I submit a separate pull request about it or combine it with the actual functionality proposed in this issue?

[pekkaklarck]

I think I got this implemented. This is the basic approach:

• ContentMatcher keeps count of how many matches are found altogether
• pss_run returns the total match count
• main() has return 0 if total_matches else 1
• main() got except Exception as err to catch possible unexpected errors
• sys.exit() is called by pss/pss.py scripts (can easily revert that back)

Is anyone using main() programmatically? If yes, I'd consider doing also the following changes to main(), possibly as a separate pull request and even as a separate issue:

• Don't call sys.exit(). This I already did.
• Don't catch exceptions. Caller can do that.
• Return the total number of matches instead of the rc.
• Possibly raise or return something special when --help or similar used.

This would obviously require adding exception catching and rc creation into pss/pss.py scripts.

Personally I'm not sure is the above a good idea or yagni. Wanted to point out possibilities while browsing through the code anyway.

[eliben]

Yes, it's fine to do sys.exit(main()) in scripts/pss.py, and make main return the actual return code instead of exiting. I think main returning 0 for matches, 1 for no matches and 2 for errors is fine.

Doing the "return code from main, exit from caller" is also OK with me.

[pekkaklarck]

Cool, this is pretty much what I've done. Doing anything more is a bit yagni and I have other things to do as well. I'll still take a look at could I create a test for this and then will create a pull request.

[eliben]

Yes, one of the benefits of not exiting directly from main is that it should be somewhat easier to test.

[pekkaklarck]

Tests revealed that my original idea of ContentMatcher keeping count of matches doesn't work because it is not used if finding only files. output_formatter is called also then, but because that is exposed to caller in pss_run, I don't want to change the API. Simple solution of using match_found boolean flag ought to be good enough.

[eliben]

Yes, sounds good. No need to complicate ContentMatcher - this is clearly YAGNI.

[pekkaklarck]

optparse using sys.exit() is super annoying, and using it both with --version and --invalid-option is plain stupid. Needed to extend optparse.OptionParser to separate between these cases.