search

lunarmodules / lua_cliargs

A command-line argument parsing module for Lua.
MIT License
123 stars 24 forks source link

Cleanup module on error #44

Closed o-lim closed 9 years ago

o-lim commented 9 years ago

Currently lua_cliargs does a cleanup after successful parsing of command-line arguments. This change performs cleanup whenever an error is encountered as well.

Tieske commented 9 years ago

I don't think I like this.

When the arguments cannot be parsed, then the user tells the application to do something, which the application doesn't understand. This is an error condition that cannot be handled, so the one logical thing to do for the application is to terminate. So there is no use in cleaning up the module.

The one thing I can figure the app to do, is try and correct the situation by itself, for some special case of arguments not supported by cliargs, and then try and parse again. In that cse you'd want the parser to still be available.

my 2cts.

o-lim commented 9 years ago

I can see where you are coming from. In the event of a parse error, the most likely action would be to exit with an error. I was just thinking about unit testing a module that used lua_cliargs internally, essentially functioning as a wrapper for lua_cliargs. In such a case it is easy enough to manually unload lua_cliargs at teardown or after_each.

o-lim commented 9 years ago

@amireh if you agree with @Tieske, then you can go ahead and close this PR. I don't really have a strong option either way.

amireh commented 9 years ago

Sorry guys, I'll take a look at these on Saturday (hopefully! :)

From a quick read and not much thought, I agree with @Tieske - but I'll revisit soon.

o-lim commented 9 years ago

@amireh any chances for a merge anytime soon?

amireh commented 9 years ago

Can't we just export the cleanup function and keep the current behavior as-is? Then, if you care, you can clean up on error explicitly (given that the rc will be nil.)

o-lim commented 9 years ago

Yeah, I'm actually not so concerned about his PR. Any chances of merging the other PRs?