search

exercism / python-analyzer

GNU Affero General Public License v3.0
8 stars 13 forks source link

Add basic CLI parsing #4

Closed yawpitch closed 5 years ago

yawpitch commented 5 years ago

After seeing some discussion on Slack I thought I'd take whack at improving the CLI interface of the Python auto-analysis tool a little.

Converts bin/analyze.py to use argparse, adding some help and basic argument parsing and validation. Will now limit the first argument to a valid EXERCISE name (ie two-fer) for which a corresponding lib/EXERCISE/analyzer.py file exists. The second argument must also be a directory that actually exists on disk.

Updates dynamic import of the analyzer.py to not use sys.path maniupulation, which was never really a "best" practice and has become more than a little problematic in Python 3.

Also updates lib/two-fer/analyzer.py with more rationalized exception handling, uses the with context manager on output, and makes the resulting JSON a little more human readable.

Not sure how widely this thing is being used yet so I've made this a WIP so I can open this up for discussion. For that reason I have not updated the README to reflect the changes described above.

TreyAguirre commented 5 years ago

@yawpitch I resolved all the merge conflicts and looked over the changes you made, looks great! I'm ready to merge this whenever if you're satisfied with it or leave it up longer if you want more feedback/discussion. Just message me on Slack or comment here when you feel it's ready.

yawpitch commented 5 years ago

Assuming everyone is ok with the directory-based validation of the exercises, I think it’s mergeable. It presumes that the [CWD]/lib/[EXERCISE]/analyzer.py structure is the one true pattern, but I could imagine that we might want to have a package per analyzer instead (ie [CWD]/lib/[EXERCISE]/init.py). That would require a little different approach to dynamic import, but in many ways a simpler one.

Eventually I’d like to add a tool or subcommand for scaffolding out a new analyzer that inherits from an ABC... seems like it’s a good idea to make building a new analyzer as easy as possible. On Jun 12, 2019, 19:01 +0100, TreyAguirre notifications@github.com, wrote:

@yawpitch I resolved all the merge conflicts and looked over the changes you made, looks great! I'm ready to merge this whenever if you're satisfied with it or leave it up longer if you want more feedback/discussion. Just message me on Slack or comment here when you feel it's ready. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.