exercism / python-analyzer

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

Issue 20: Exercise Names + A Refactor to Bring Into Spec Compliance #22

Closed BethanyG closed 3 years ago

BethanyG commented 3 years ago

As stated on the πŸ₯« , a refactor to bring the analyzer "up to spec" 🌈 ✨ :

Also added CI-related files.

More work is needed to clean up code and better integrate PyLint, but this is a functional first pass. πŸŽ‰

BethanyG commented 3 years ago

@iHiD -- Thank you for the comments (and the spelling patrol!!)! πŸ˜„

One thing I'm not quite able to work out from a cursory glance is what happens if there are a mixture of comment types for one solution? If for example there is a actionable and a informative comment, what does the top-level message do?

The "highest priority" comment takes precedence in that circumstance, and a a DIRECT ("There are a few changes we suggest that can bring your solution closer to ideal." -- which maps to ACTIONABLE comments) summary message would be returned. The logic is in a @classmethod on Analysis -- starting at line 78 in analysis.py.

Frankly, this was me not (quite) wanting to throw away the old APPROVE/DISAPPROVE organization - so I turned it into a summary. Not sure I am happy with that, and I might get rid of it as I proceed down the road to integrating pylint better. Open to thoughts/suggestions/ideas on that front.

I did add a summary status of GENERIC, for when there were no exercism specific comments, and only pylint returned anything. Haven't wired that up yet. The comment Enums are in comment.py

One thought is to write a "generic" analyzer (with that GENERIC summary message) that ran a student's solution through pylint only, so that we could at minimum have a pylinting for every exercise. Then double back and write/customize individual analyzers for exercises as we get to them. Open to suggestions on that as well. Pylint uses a pretty nice lib for AST wallking (astriod), that I'm eager to get my hands on. πŸ˜‰

iHiD commented 3 years ago

Thank you for the comments (and the spelling patrol!!)! πŸ˜„

Of course πŸ™‚

Open to thoughts/suggestions/ideas on that front.

I've not done this yet for the Ruby one, but my (medium-term) plan is to add summary comments where I feel they add some explicit extra value. The website itself should handle informing the student what to do in general (ie "please sort this before proceeding") so the aim of this field was for the analyzer to be able to give extra information where that wasn't enough. That said, in the short-term I'll probably do exactly the sort of thing you've done here. One to explore further down the line for sure πŸ™‚

BethanyG commented 3 years ago

@cmccandless @iHiD @SleeplessByte -- are there additional points I need to address here for approval? Just checking....

iHiD commented 3 years ago

(Not from me, but you don't need my code-level-approval anyway as this doesn't touch anything I'm a codeowner of. I'll approve so you can merge when you're ready πŸ™‚)

iHiD commented 3 years ago

Whoop πŸŽ‰