Source code modernization

[vergenzt]

Hi @nedbat! I've been thinking about a few suggestions / requests for this app, but you've mentioned in a few places that the code style is pretty archaic. I'm interested in helping to modernize it. I'm gonna put my tentative todo list here -- obviously :+1:'ing or :-1:'ing each PR will be up to you, but if you're up for weighing in here that'd be helpful.

• [x] Auto-format Python code using ruff format (preferably via pre-commit hook?)

  • ~~Note: I may go ahead and submit a PR for this one since it's trivial to do.~~
  • https://github.com/nedbat/cog/pull/33

• [ ] Lint Python code using ruff check (preferably via pre-commit hook?) & apply suggestions

  • Some of them are more trivial, but I tend to think it's worth it for the consistency. 🤷

  • Here's the current list I'm seeing:

    23 "errors"

    cogapp/__init__.py:7:21: F401 `.cogapp.Cog` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
    cogapp/__init__.py:7:26: F401 `.cogapp.CogUsageError` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
    cogapp/__init__.py:7:41: F401 `.cogapp.main` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
    cogapp/cogapp.py:115:27: E741 Ambiguous variable name: `l`
    cogapp/cogapp.py:118:25: E741 Ambiguous variable name: `l`
    cogapp/cogapp.py:128:58: E741 Ambiguous variable name: `l`
    cogapp/cogapp.py:129:56: E741 Ambiguous variable name: `l`
    cogapp/cogapp.py:161:9: E722 Do not use bare `except`
    cogapp/cogapp.py:429:13: E741 Ambiguous variable name: `l`
    cogapp/cogapp.py:446:21: E741 Ambiguous variable name: `l`
    cogapp/cogapp.py:473:21: E741 Ambiguous variable name: `l`
    cogapp/cogapp.py:492:25: E741 Ambiguous variable name: `l`
    cogapp/cogapp.py:504:17: E741 Ambiguous variable name: `l`
    cogapp/cogapp.py:525:21: E741 Ambiguous variable name: `l`
    cogapp/cogapp.py:568:21: E741 Ambiguous variable name: `l`
    cogapp/cogapp.py:573:25: E741 Ambiguous variable name: `l`
    cogapp/cogapp.py:577:17: E741 Ambiguous variable name: `l`
    cogapp/cogapp.py:704:13: E741 Ambiguous variable name: `l`
    cogapp/test_makefiles.py:29:16: E721 Do not compare types, use `isinstance()`
    cogapp/utils.py:47:9: E741 Ambiguous variable name: `l`
    cogapp/whiteutils.py:45:9: E741 Ambiguous variable name: `l`
    cogapp/whiteutils.py:47:13: E741 Ambiguous variable name: `l`
    cogapp/whiteutils.py:49:13: E741 Ambiguous variable name: `l`
    

• [ ] Convert CogOptions from getopt to argparse

  • I'd probably extract the options & option parsing into a separate cogapp/options.py file. And maybe make the usage string autogenerated based on the configured argument help strings?

• [ ] TBD...

[nedbat]

I have no objections to those changes, I just don't feel much need to make them. I don't use pre-commit hooks, but if we add the ruff checks to CI also, then we can stay in-synch.

For the formatting changes, we should add a .git-blame-ignore-revs file also so that git blame won't get distracted by them.

What substantive changes were you thinking of making? I'd hate for you to put in this work and then for us to disagree on your eventual goal.

[vergenzt]

Didn't have much specifics in mind. Mostly was just bored and love this tool and wanted to unblock some previous requests you've mentioned. E.g. 5 years ago you mentioned the coding style is "really archaic". 😆

I think some things I would love to do include:

1. Add long option equivalents for some of the short options currently available. (I prefer to use long options in source controlled scripts for readability's sake.)

2. I think my biggest long term goal would be https://github.com/nedbat/cog/issues/17#issuecomment-1919632924

   I'm currently interested in creating an offshoot of this package called cogshell (where the code snippets are shell code instead of Python 🙂). I would love to use this package's code as the processing engine, but it needs a bit of refactoring for that to work.

   I've had the thought that maybe there's a world in which that mode of evaluation could possibly live within this cog package as an option flag? Or maybe a different opening marker? I'm not sure. 🤷 I think if it ends up being an offshoot package that's fine too though -- in either case I think cleaning this package up to be able to inherit & override specific parts would be more productive than rewriting the core logic from scratch in an offshoot package.

I'll harbor no expectations that you'll approve a future changes just because you approve an earlier one. 🙂

[vergenzt]

For the formatting changes, we should add a .git-blame-ignore-revs file also so that git blame won't get distracted by them.

This is totally awesome by the way. I had no idea this is a thing but I love it.

[LecrisUT]

To share my perspective, doing such refactoring does pay out as external contributors stumble upon this and are used to some workflows. I have had various setbacks due to the IDE formatting in a different way than the other code. If there is a pre-commit, this advertises what coding standards are used and will help us adapt faster.