Refactoring, Code Styling and CI integration

Oct 14, 2011 at 9:29 PM


We're using PTVS on a largish Python web fronted project with a bunch of C# on the backend. We love having everything in one IDE.

We just RTMed our v1 and are about to start a major refactoring stage, as well as setting up continuous integration and continuous deployment for the product.

We would like to develop a system where we have a set of naming/style rules that are integrated into our IDE experience as well as executed on the CI server as part of our pass/fail evaluation for source commits.

We've looked at pylint/pep8, and are familiar with the refactoring tools provided by PTVS. 

What we get now, if we setup pylint as a external tool, it will run the entire file, and give a full report in the output window, and you can double click on the output line, and navigate to the problem area. This is not bad, but we'd love to see it more tightly integrated to give feedback as you edit, similar to the experience you get using ReSharper in C#. It'd be nice to be able to do a Refactoring->CleanUp Code that enforced pep8 standards, and gave nice little in-line popup widgets for performing those functions as refactorings. Ideally it'd be the same ruleset/code on the CI server. 

Can anyone recommend the best way to get a seamless experience for this stuff? Is this something we'd have to start hacking ourselves or has anyone gotten that far yet?

Thanks, Troy

Oct 14, 2011 at 9:45 PM

You can vote on the following features which are all related:   [something like PyLint, but based upon our analysis]  [PyLint integration] [add reformat code command]

We're not planning on implementing any of those features in the next release or two so if this was something you really wanted, and were willing to implement, you could implement it and we are willing to take the contributions back. 

I'd think of matching PEP8 guidance in two ways: First is the basic formatting, when and where there should be spaces, etc...  That's like feature 176.  We already have code for taking a Python AST and converting it back into strings AppendCodeString methods on the AST nodes in Microsoft.PythonTools.Parsing.Ast).  Supporting reformatting would basically just be inserting the "correct" white space instead of using the verbatim white space which was persisted in the tree.  This would then get wired up to the Edit->Advanced->Format Document/Edit->Advanced->Format Selection commands.

But then there's also naming guidelines which will require real refactorings if you want to do something like rename a camel cased method to have underscores or rename a class to be pascal cased.  That'd be something like a smart tag (probably easier to plug into that then adding a new widget) to offer to do the rename. 

And of course integrating PyLint could be pretty easy - it should just be adding a new command which shells out to PyLint, collects the messages, and adds them to the VS error list using the IVsErrorList interface and also use a SimpleTagger<ErrorTag> to add the squiggles into the open editor buffers (you can see examples of both of these in ProjectAnalyzer.cs).  You could even make it run automatically in the background (on file save?  Or after a buffer has been idle for a while like how we kick off re-analysis of the files?) so that it all happened automatically.

If you are interested in implementing these into PTVS let me know and I'll be willing to answer any questions or help w/ any problems you run into.




Oct 14, 2011 at 10:59 PM

Sweet. We'll see if we can get the pylint integration working and submit a patch. 

Are these the most recent/accurate build instructions? 

Oct 14, 2011 at 11:02 PM

Yep, those should be up to date, let us know if you have any problems.

Oct 18, 2011 at 6:50 PM

Thinking about this a bit, wouldn't it make sense to avoid the processing overhead of pylint/pep8? It seems like a lot of unnecessary work to have those scripts go through parsing again when PTVS already has sophisticated parsing, with error reporting that happens at appropriate times. It seems like I could just extend the analysis that's already happening via AnalysisQueue and add some additional business rules for naming conventions and what not. This would be a much better user experience than dealing with the lag of running something external. 

Supposing I went that route, what's the appropriate place to add that kind of code?

Seems like the error reporting is happening in Parser and PythonNameBinder mostly. I think I could create a similar class that implements PythonWalker and add an optional "style analysis" stage to ProjectAnalyzer.ParseOneFile reusing the same ErrorSink/etc. Then just implement node-type specific style validation in that walker, reporting warning style errors via the existing ErrorSink. At minimum it could just run the regexes and report things that don't match (pylint already has nicely laid out the REs on their docs page here:

Does this sound reasonable? 

Main downside to that is that as pylint gets upgraded someone would have to maintain the implementation, vs just updating the script and working with it's output.

Thanks, Troy


Oct 18, 2011 at 6:57 PM

That approach is perfectly fine - that'd be similar to starting work on #1 but in this case just dealing w/ style rules.  There might be some desire to customize the style rules but that could always be added later as people make various requests.

Oct 18, 2011 at 10:56 PM

Oh, and where to add the code...  I think subclassing PythonWalker and doing this as a seperate pass is the right way to go.  I think doing this in ProjectAnalyzer also makes the most sense because it is already doing the warning/error additions - so you probably want to do it in ParseFile and ParseBuffers, and these 2 functions could probably stand to have some of the code between them unified as well.

Oct 18, 2011 at 11:01 PM

This is exactly what I've done, and it's working. Yay! 

So far, I've got some basic regex matching for various node names and multiple imports per lines checked, etc.. I'm in the rules implementation phase now, and hopefully will have a patch to submit very soon. 

Thanks, Troy