Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The redundancy of argument parser (possibly its removal) in the class gunicorn.app.wsgiapp.WSGIApplication from file wsgiapp.py #3163

Open
Humbulani1234 opened this issue Feb 29, 2024 · 1 comment

Comments

@Humbulani1234
Copy link

1. Argument removal description:

The method self.init defined in the class gunicorn.app.wsgiapp.WSGIApplicaton and reproduced below:

def init(self, opts, parser, args): # removed parser
        self.app_uri = None

        if opts.paste:
            from .pasterapp import has_logging_config

            config_uri = os.path.abspath(opts.paste)
            config_file = config_uri.split('#')[0]

            if not os.path.exists(config_file):
                raise ConfigError("%r not found" % config_file)

            self.cfg.set("default_proc_name", config_file)
            self.app_uri = config_uri

            if has_logging_config(config_file):
                self.cfg.set("logconfig", config_file)

            return

        if len(args) > 0:
            self.cfg.set("default_proc_name", args[0])
            self.app_uri = args[0]

does not seem to require the parser argument. This method is called from the class gunicorn.app.base.Application by the
load_config method, reproduced below:

def load_config(self):
        # parse console args
        parser = self.cfg.parser()
        args = parser.parse_args()

        # optional settings from apps
        cfg = self.init(args, parser, args.args) # removed parser
                                                                                                                                                                                                                                                                                
        # set up import paths and follow symlinks
        self.chdir()

        # Load up the any app specific configuration
        if cfg:
            for k, v in cfg.items():
                self.cfg.set(k.lower(), v)

        env_args = parser.parse_args(self.cfg.get_cmd_args_from_env())

        if args.config:
            self.load_config_from_file(args.config)
        elif env_args.config:
            self.load_config_from_file(env_args.config)
        else:
            default_config = get_default_config_file()
            if default_config is not None:
                self.load_config_from_file(default_config)

        # Load up environment configuration
        for k, v in vars(env_args).items():
            if v is None:
                continue
            if k == "args":
                continue
            self.cfg.set(k.lower(), v)

        # Lastly, update the configuration with any command line settings.
        for k, v in vars(args).items():
            if v is None:
                continue
            if k == "args":
                continue
            self.cfg.set(k.lower(), v)

        # current directory might be changed by the config now
        # set up import paths and follow symlinks
        self.chdir()

specifically, it is called at this line: cfg = self.init(args, parser, args.args).

I've run Gunicorn tests and specifically the file tests.test_config.py and all the tests before and after the
removal of the argument parser are the same. Therefore, is there any particular reason for this argument to be included?

3. Tests:

platform linux -- Python 3.10.12, pytest-8.0.0, pluggy-1.4.0
configfile: pyproject.toml
plugins: cov-4.1.0
collected 251 items  

....

---------- coverage: platform linux, python 3.10.12-final-0 ----------
Coverage XML written to file coverage.xml

$\large\color{green}{\textsf{=================== 251 passed in 6.98s =============================}}$

4. Environment:

python --version $\large\color{red}{\textsf{|}}$ Python 3.10.12

OS specifics:

OS: Ubuntu 22.04 jammy
Kernel: x86_64 Linux 6.5.0-21-generic

Virtual environment

asgiref==3.7.2
blinker==1.7.0
click==8.1.7
Django==5.0.2
dnspython==2.6.1
eventlet==0.35.2
Flask==3.0.2
gevent==24.2.1
greenlet==3.0.3
gunicorn @ git+https://github.com/benoitc/gunicorn.git@88fc4a43152039c28096c8ba3eeadb3fbaa4aff9
hupper==1.12.1
itsdangerous==2.1.2
Jinja2==3.1.3
MarkupSafe==2.1.5
packaging==23.2
Paste==3.7.1
PasteDeploy==3.1.0
plaster==1.1.2
plaster-pastedeploy==1.0.1
pyramid==2.0.2
six==1.16.0
sqlparse==0.4.4
translationstring==1.4
typing_extensions==4.10.0
venusian==3.1.0
WebOb==1.8.7
Werkzeug==3.0.1
zope.deprecation==5.0
zope.event==5.0
zope.interface==6.2
pytest==8.0.2
pytest-cov==4.1.0

End

@pajod
Copy link
Contributor

pajod commented Apr 26, 2024

True, but gunicorn in general lacks a distinction between public API (that downstream users can trust to not go away without notice) and internal (preferably underscore-prefixed) API. This one is rather clearly the former case though, being in the parent class and thus reasonably copied to any custom overrides.

I'd suggest approaching cleaning up such oddities from two separate vectors:

  1. add annotation stubs (.pyi) and inline them (after dropping 3.7 compat) and later, so people have an easier time adapting their code using static type checkers
  2. internally, prefer keyword arguments wherever copying the full method signature is not desirable - while retaining compatibility with custom code still using positionals, emitting DeprecationWarning

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants