nodeca / argparse

CLI arguments parser for node.js. JS port of python's argparse module.
Other
487 stars 73 forks source link

Option to have more control over help and version flag (opt out of exit & choosing when to log) #113

Closed AKST closed 4 years ago

AKST commented 7 years ago

I will write a PR, if this is a desired feature as I plan on maintaining a fork of my own that does this, but I was wondering if there was any desire for a feature that opted out of exiting on the help command, for say additional clean up or if you have async stuff running while the args parsing that you want clean up if not required.

On top of that I would also like to opt out of logging of help info immediately after the parsing finishes, as the option exists to manually log this information, that way I can choose where it's logged too (stderr or stdout).

While I can see the exit behaviour and the logging being a default is useful for many people, I personally would like to have more control over the exit behaviour of my program and what's writing to stdout/stderr.

puzrin commented 7 years ago

I don't know what happens now in mainstream, but main principle is to "be the same and to not deviate". It's better to start with explanation of what python's argrapse suggests about this.

AKST commented 7 years ago

It's better to start with explanation of what python's argrapse suggests about this

I don't believe the python argparse implementation even offers this feature, I couldn't find anything in the documentation about it. I did spend several hours reading it, but I may have missed it.

but main principle is to "be the same and to not deviate"

By this do you mean, the general philosophy of this library? Because I did notice this. I guess if the absolutest end goal of the library is to match the python API bit for a bit, a feature like this may not be a priority, or may possibly conflict with this libraries goal.

In the same breath this kind of feature doesn't modify any existing features of the library, so any existing apps that depend on this library shouldn't break, and it should be completely opt in by supplying an additional argument, something like this.

import { ArgumentParser } from 'argparse';

const parser = new ArgumentParser({
  // default value of exit should be true
  canExit: false,
  // default value should also be true
  canLog: false,
});

And the if someone specifies the the help command and they wish to tidy up some preemptive async code that was no longer needed, they can call tidy that up before


My own motivations for having a feature like this is so I don't have to monkey patch the library to disable this behaviour of the library, hence me considering choosing maintaining my own fork. Also it's not something I've seen in other node libraries, well at least the process.exit, they normally will specify via callback or a return value that program should exit instead of killing the process.

Also in my mind, parser.parseargs indicates it will return a parsed result not terminate the program, possibly throw an exception on invalid input.

WORMSS commented 7 years ago

Wonder if you can talk the original python argparser to implement your upgrades... Then they would be obliged to implement it in the node version.

AKST commented 7 years ago

Sure, but I'm not even sure the python one is still being actively maintained (judging by the last commit), and I'm not entirely sure what the merit of having a 1:1 API match with the python API is.

Like I get that someone who is familiar with the python library can pick this library up quite fast, but should that really come at the expense of adding features to this library?

Edit: I'll ask, but it's a pretty roundabout way of evaluating this functionality, and it might be possibly weird from the perspective of the python library author to be asked to consider implementing a feature, so another library in a seperate language can consider implementing this.

Or are you suggesting asking their input on how they would implement this?

AKST commented 7 years ago

Actually I'll just mention them here instead.

@ThomasWaldmann as the maintainer of the python equivalent of this library, what are your thoughts on a piece of functionality that provides the consuming application the ability to opt out of the default logging and exiting behave of parser.parseArgs?

Do you have any input on how such functionality should be implemented?

ThomasWaldmann commented 7 years ago

Python's argparse is maintained as part of the standard library, by the Python core developers - not by me.

I just maintained the python 2.6 compatible backport, but python 2.6 should be rather dead now.

AKST commented 7 years ago

@ThomasWaldmann right my mistake, your library was the first one that came up on github, but that does explain what the last commit dates back to 2015

puzrin commented 7 years ago

By this do you mean, the general philosophy of this library? Because I did notice this. I guess if the absolutest end goal of the library is to match the python API bit for a bit, a feature like this may not be a priority, or may possibly conflict with this libraries goal.

That's a port. It should be 100% equal to main project in ideal world. Then you can skip maintaining local docs and reduce efforts for support. Port can not be 99% or "with my improvements because i know better than authors how to write code".

Acceptable exclusions are related to fundamental language differences and coding approaches. When exact copy is technically impossible. I know, we did some mistakes and this port is not as accurate as it should be, but we have to fix that (see issue about 2.0.0).

In other words, all deviations are rejected by default, but we are not blind robots :). If author understand what is "port" and has serious reasons for deviation, there is a chance to accept it.

AKST commented 7 years ago

That's a port. It should be 100% equal in ideal world. Then you can skip maintaining local docs and reduce efforts for support.

That's fair enough, if this is a 1:1 port then I might just maintaining my own fork then, as I mostly asked about this here incase it was worth sharing my changes back.

Port can not be 99% or "with my improvements because i know better than authors how to write code".

I don't feel that strongly about my changes being accepted as I do accept other people have their own preferred approaches to these thing, it less "i know better than authors how to write code", and more the fact it's less maintenance for me, but I would rather fork in the event my changes not being accepted than my application continue to monkey patch the library.


But for what it's worth sys.exit throws an exception instead of terminating the process, as seen here (but that is because sys.exit throws an exception). So the following python code allows you to continue your program if you want to have more control over the program and exit more gracefully, instead of enforcing an architectural constraint on the consuming application.

#!/usr/local/bin/python3
from argparse import ArgumentParser

try:
    parser = ArgumentParser(add_help=True, description='lol')
    result = parser.parse_args()
    print(result)
    print('this did run')
except BaseException:
    print('oh look an error')

Unfortunately the error doesn't tell you much about the result of the parser.

Acceptable exclusions are related to fundamental language differences and coding approaches. When exact copy is technically impossible.

You could argue that sys.exit is a very python way of doing things, where as I feel personally it's not as natural for a library in node to terminate someone else's application, I personally feel it's more natural in a node API to tell the program it should exit then exit for them.

But that's more of a convention than a fundamental language differences and it's also debatable, but that is my strongly held opinion from my experience using other peoples libraries. Like Generally I find unless it's a framework, good libraries don't enforce architectural constraints on the applications utilising them.

AKST commented 7 years ago

In other words, all deviations are rejected by default, but we are not blind robots :). If author understand what is "port" and has serious reasons for deviation, there is a chance to accept it.

That's fair enough, and I understand since that's the general design philosophy of the library 😄

But as a final possibly futile attempt to see if there's any wiggle room to modify the of behaviour exiting the process, in what i mentioned in my last comment?

If it doesn't fit the criteria that's okay, as my application isn't your responsibility and I'll just make a fork for my own purposes.


edit feel free to close the ticket, if it doesn't fit the criteria

puzrin commented 7 years ago

@AKST there were several issues about the same problem. And i understand that something should be changed if such feedback presents. I would accept improvement, but need to understand how to do it right.

If you have time, i'd suggest to do this steps:

If good (in terms of our approach) solution possible, it will be accepted. Deviation will be accepted too if it will not become PITA in future.

puzrin commented 7 years ago

As far as i remember #105 was related. I hoped to solve issue in 2.0, but process postponed for unknown time.

To be clear: in current state arparse is good enougth for my needs, side effect is that i use refactoring to 2.0 as test case to hire people :). In other case it would be more simple to reserve existing human resourses and fix everything at once in one week. But i see that time goes and nothing moves forward. So, i will accept PRs from anyone who understand that port should be close to original.

The general list of problems is known and recorded in #112. Just keep in mind to not create more problems there.

AKST commented 7 years ago

Sorry for the slow reply just caught up with study/work, but thanks @puzrin!

I'll have a look at #112 and as it seems to be blocking the PR for #100 I might leave a comment asking a few questions.

In addition to what you've said, I might start by messing around with my fork till I get something that feels ergonomic, as my priority is getting my own application in a working state :smile: but since I still want to help where I can after that I'll start trying to align it with more with the original.


re: issue #100

So my understanding of issue #100 is that if you set a debug flag, can stop this argparse from exiting via process.exit(<number>), and instead throw an exception, correct?

And PR #105 is intended to fix this?

puzrin commented 7 years ago

As far as i understant, that was attempt to avoid process.exit() at last in debug mode, keeping compatibility in normal mode. But in fact, that's a kluge. Better than nothing but not good in long term.

gprossliner commented 4 years ago

I needed this in a jasmine test, which obiously should not exit the runner. So I assigned process.exit. So you can assert on the return code.

describe("argparse", () => {

    let retcode
    let orgexit

    beforeEach(()=>{
        orgexit = process.exit
        process.exit = nr=>retcode = nr
    });

    afterEach(()=>{
        process.exit = orgexit
    })

    it("should return 2 on help", ()=>{
       parseArgs();
       expect(retcode).toBe(2);
    })
}

If you would like to return an different code on --help, you may:

orgexit = process.exit
process.exit = orgexit(nr==2 ? 3 : nr)

One thing I forgot: If you wanna control the output, you need to overwrite process.stdout.write and process.stderr.write.

rlidwka commented 4 years ago

If you want to stop argparse from exiting, override its exit function (in subclass or just monkey-patch it). Keep in mind that it should either throw or exit (it shouldn't return control to caller).

class MyParser extends argparse.ArgumentParser {
    exit() {
        throw 'EXIT'
    }
}

const parser = new MyParser()
try {
    parser.parse_args('-h')
} catch (e) {}
console.log('error intercepted')

This is how it's supposed to be done in python, and it should work here as well.

rlidwka commented 4 years ago

Oh, forgot to mention, python has exit_on_error flag for ArgumentParser, which has been ported here as part of argparse 2.0.

Maybe you'll have success using it, although it's a bit broken for now. If you find any issues, please bug python devs to fix it over there, and we'll port their changes.