Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Anti8 #30

Closed
wants to merge 13 commits into from
Closed

Anti8 #30

wants to merge 13 commits into from

Conversation

pmaupin
Copy link
Collaborator

@pmaupin pmaupin commented May 17, 2015

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

@pmaupin
Copy link
Collaborator Author

pmaupin commented May 18, 2015

Working AFAIK -- please review

@berkerpeksag
Copy link
Owner

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

else:
result.append('%s%s' % (indent, repr(node)))

result = []
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make the code more readable we can move this to top of the function block.

@pmaupin
Copy link
Collaborator Author

pmaupin commented May 18, 2015

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
Copy link
Owner

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 :)

berkerpeksag and others added 4 commits May 19, 2015 20:37
  - Rebased against master
  - Renamed from anti8 to rtrip
  - Defaults to round-tripping the stdlib
  - Shows usage if bad parameters given
  - Creates files with AST dumps if round-tripping fails
@pmaupin
Copy link
Collaborator Author

pmaupin commented May 20, 2015

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

@berkerpeksag berkerpeksag modified the milestone: 0.6 May 22, 2015
@pmaupin
Copy link
Collaborator Author

pmaupin commented May 24, 2015

@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
Copy link
Owner

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

@pmaupin
Copy link
Collaborator Author

pmaupin commented May 24, 2015

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
Copy link
Collaborator Author

pmaupin commented May 26, 2015

I have merged this into the pm_develop branch

@pmaupin pmaupin closed this May 26, 2015
@pmaupin pmaupin deleted the anti8 branch May 26, 2015 02:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants