search

berkerpeksag / astor

Python AST read/write
https://pypi.org/project/astor/
BSD 3-Clause "New" or "Revised" License
810 stars 102 forks source link

Anti8 #30

Closed pmaupin closed 9 years ago

pmaupin commented 9 years ago

Here is the function you asked for in issue #29 and the version of anti8 using the mechanism we discussed.

pmaupin commented 9 years ago

Working AFAIK -- please review

berkerpeksag commented 9 years ago

Looks good to me in general, I'll add some nitpick comments :)

pmaupin commented 9 years ago

Thanks! I'll look into those. In fact, the dump will be handled completely differently, because it sucks, and I have a better one somewhere -- just need to find it.

To be honest, I wasn't really ready to release this yet (possibly shouldn't have done a pull request -- I'm still learning about this), but it was so helpful in finding bugs I wanted to get it on a branch where others could use it.

I also really appreciate the nitpicks -- I've been out of the loop on doing stuff publicly for awhile, so those (and the automatic doc build, etc.) are really good learning for me. I need to git up to speed to deal with a few other projects that I am migrating (and need to migrate) from google.

Thanks, Pat

berkerpeksag commented 9 years ago

To be honest, I wasn't really ready to release this yet (possibly shouldn't have done a pull request -- I'm still learning about this), [...]

It's actually a good practice and I'm doing it all the time :) I'll probably keep pinging you to review my branches/pull requests in the future :)

pmaupin commented 9 years ago

Berker:

OK, after several diversions, I finally got to the tool that I wanted for checking PEP8 edits.

I think I took most of your suggestions. I didn't use argparse, because it felt like using an elephant gun to shoot a gnat (maybe that's because I don't use it very often), but it wouldn't break my heart if somebody cleaned that up later, either.

Obviously, there is a lot more work that can be done, including better pretty-printing, and better handling for round-tripping unicode source code, and a revamping of the unit-tests to autogenerate some tests, but unless there are glaring issues, I'm done with this for now, so please review my two branches and merge/cherry pick them into the master if they seem reasonable.

Thanks, Pat

pmaupin commented 9 years ago

@berkerpeksag

This is ready to merge. It seems to work OK, and I keep having to add it in when I'm testing other branches. Since I'm not the greatest at git, unless you have serious problems with it, I'd really like to merge it to simplify my life.

Thanks, Pat

berkerpeksag commented 9 years ago

I'll take a look at both of your pull requests tonight. Thanks!

pmaupin commented 9 years ago

That'll be awesome.

There are always more optimizations (e.g. there are a few places where I still have parentheses to get rid of) but I think the basic approach is sound, so those can be separate small merges later.

Thanks, Pat

pmaupin commented 9 years ago

I have merged this into the pm_develop branch