Refactors (including optparse → argparse), and add long options #35
Conversation
Except for: - `sOut`, for backwards compatibility, since it's included in public API Via VSCode find and replace of: - patt: \b[bs](?!File)([A-Z]\w+)(?<!sOut)\b with: \l$1 - patt: \b[bs]File([A-Z]\w+)(?<!sOut)\b with: fileName$1 - patt: sFile with: fname - patt: \bf([A-Z]\w+)\b with: file$1
Purposes: 1. To match typical modern Python coding styles (recommended by PEP 8) 2. To prepare for argparse usage, which uses snake_case for default dests Via https://ast-grep.github.io/, with rule: ``` id: _ language: python rule: pattern: $ID kind: identifier all: - regex: '^[^A-Z]' # not PascalCase - regex: '[a-z][A-Z]' # has camelCase transition not: any: # backcompat (part of cog module API) - regex: ^sOut$ - inside: kind: attribute has: field: object regex: \.cogmodule$ # unittest methods - regex: ^assert - regex: ^setUp$ - regex: ^tearDown$ - regex: ^addCleanup$ transform: ID_SNAKE: convert: source: $ID toCase: snakeCase UNDERSCORE: replace: source: $ID replace: '^(_)?.*' by: '$1' fix: $UNDERSCORE$ID_SNAKE ``` plus manual review of all changes
vergenzt
force-pushed
the
gh-32-optparse-to-argparse
branch
2 times, most recently
from
917027b
to
f5dc07c
Compare
vergenzt
force-pushed
the
gh-32-optparse-to-argparse
branch
from
f5dc07c
to
f98308f
Compare
|
Wow, this is a lot. I know how these kind of refactors go, they kind of snowball. It would be easier to review in smaller pieces, but that might not be possible. |
|
Yeah sorry for the shock! It's currently split up commit-by-commit, but I can split it into separate PRs for ease of review 👌 |
|
Closing in favor of separate PRs |
|
Followup PRs are going to be
Unfortunately per https://github.com/orgs/community/discussions/4477 there doesn't appear to be a way to create dependent PRs. I'll create "draft" PRs within my fork in case you want to comment on them all at the same time, otherwise I can open PRs one at a time. Again, if you don't like any of the changes for any reason it will not hurt my feelings if you reject any of them. 🙂 I'm doing this mainly for fun. |
Hi! I had fun with this one. 🙂 Let me know if you prefer that I break out separate PRs for the individual refactor commits. I kept them in one PR for now since the "business value" for the first two isn't as clear until you get to the point of adding long options (where using
snake_casegets you a free implicitdestparameter). Though IMO the majority of the value is in consistency with broader Python ecosystem conventions.(Which, admittedly, is funny to see alongside the
camelCaseAPI for the agingunittestmodule. 🤦🏻♂️ I still think it's worth usingsnake_caseoverall outside of those API calls tho, personally.)If you object to any part of this that's totally fine - I had fun and learned a bunch doing these so the effort wasn't wasted regardless. 🙂 I'm happy to make any changes you prefer. I made a few executive decisions along the way, any of which you, as the maintainer, are of course welcome to disagree with and request changes to.