nodeca / argparse

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

fix: correctly show long strings in help output #144

Closed foxxyz closed 4 years ago

foxxyz commented 4 years ago

This is a fix for #141, which I ran into today as well.

Current behavior causes very long help strings not to be displayed in help output; instead, a duplicate of the previous line is shown (see #141).

This fix addresses that - a test has been included for verification also.

Thank you for this great library!

foxxyz commented 4 years ago

@puzrin Any feedback or objections to getting this merged?

Let me know and thanks!

foxxyz commented 4 years ago

@puzrin: gentle bump to see if you've had time to take a look... would appreciate it, thanks!

puzrin commented 4 years ago

@foxxyz we started (really) full rewrite of argparse, doing more fresh and more close port. At the end all issues and PRs will be rechecked at once.

foxxyz commented 4 years ago

Sounds good! Thank you for the response and looking forward to it!

rlidwka commented 4 years ago

full rewrite done, bug has fixed itself

foxxyz commented 4 years ago

@rlidwka Nice work! Happy to see the update, although I'm disappointed with reverting back to snake_case since the general standard JS practice mainly uses camelCase.

Any possibility you would accept dropping deprecation and allowing both aliases?

puzrin commented 4 years ago

Any possibility you would accept dropping deprecation and allowing both aliases?

Probability of this is about zero.

  1. It's impossible to guarantee correct work of aliases, except simple cases.
  2. Having JS recommendations does not mean those should be followed blind.

Deprecation messages are to simplify migration, not for production use.

foxxyz commented 4 years ago

Fair points and appreciate the response.

Thank you for all your hard work!

rlidwka commented 4 years ago

Happy to see the update, although I'm disappointed with reverting back to snake_case since the general standard JS practice mainly uses camelCase.

There are many style guides for javascript, all of them claiming to be "the standard JS practice". This one is for example literally called "standard js": https://standardjs.com/ - but its very different from the one you linked.

In this particular module snake_case was chosen for the following reasons:

It's possible to support both as we do now, but it comes with its costs:

Its possible to solve all these, but is it really worth it to do so?

If you care about it, maybe you can make a generic library that takes a module (any module) and wraps it with different letter casing? It wont solve all issues, but at least people using it will be more concious about these issues.

PS: Here's a small wrapper that partially solves this. It didn't make it into actual release version (too hard to debug), but maybe you'll find it useful:

// decorator that allows class methods to be called with both snake_case
// and camelCase throughout the entire inheritance chain
function _camelcase_alias(_class) {
    let old_prototype = _class.prototype
    _class.prototype = new Proxy(old_prototype, {
        get(target, property, receiver) {
            if (typeof property !== 'string') return Reflect.get(target, property, receiver)
            while (target) {
                if (Object.hasOwnProperty.call(target, property) || typeof property !== 'string') {
                   return Reflect.get(target, property, receiver)
                }
                let try_snake_case = property.replace(/[A-Z]/g, c => '_' + c.toLowerCase())
                if (try_snake_case !== property && Object.hasOwnProperty.call(target, try_snake_case)) {
                    return Reflect.get(target, try_snake_case, receiver)
                }
                let try_camel_case = property.replace(/_[a-z]/g, c => c[1].toUpperCase())
                if (try_camel_case !== property && Object.hasOwnProperty.call(target, try_camel_case)) {
                    return Reflect.get(target, try_camel_case, receiver)
                }
                target = Object.getPrototypeOf(target)
            }
            return undefined
        }
    })
    return _class
}

argparse.ArgumentParser = _camelcase_alias(argparse.ArgumentParser)
foxxyz commented 4 years ago

Wow, thank you for the very detailed response and reasoning.

I just read most of the source and did not realize how much of the original Python source is being emulated. I always understood the API was consistent with Python's argparse, but not that the source itself mimics it so closely (including helpers for native python keywords!). In that context, your decisions make a lot more sense and it's understood aliasing creates a lot of headaches for future upstream changes.

I'm not sure whether I fully agree but I absolutely admire the effort and idea. Thank you again.