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

Source code modernization #32

Open
1 of 4 tasks
vergenzt opened this issue Apr 25, 2024 · 3 comments
Open
1 of 4 tasks

Source code modernization #32

vergenzt opened this issue Apr 25, 2024 · 3 comments

Comments

@vergenzt
Copy link
Contributor

vergenzt commented Apr 25, 2024

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 👍'ing or 👎'ing each PR will be up to you, but if you're up for weighing in here that'd be helpful.

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

  • 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
Copy link
Owner

nedbat commented Apr 25, 2024

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
Copy link
Contributor Author

vergenzt commented Apr 25, 2024

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 Make cog usable as a library #17 (comment)

    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 added a commit to vergenzt/cog that referenced this issue Apr 25, 2024
@vergenzt
Copy link
Contributor Author

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.

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