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

Code style validation for Python using YAPF #12

Merged
merged 1 commit into from
Oct 18, 2017

Conversation

robertodr
Copy link
Contributor

This PR enables the use of YAPF for Python code style validation. Have a look here for an example.

Some questions:

  1. The format for passing lines to reformat is slightly different between clang-format and yapf:
    • The former can be invoked with clang-format -lines=start:end
    • The latter can be invoked with yapf --lines=start-end

I have inserted a check on the validator name in resolve_changes to get the correct argument set up for the actual call to the validator. This is not really clean, @nikolaykasyanov do you have suggestions on how to possibly improve on this?
2. I have put a test "Accepts a validator other than clang-format". Is that enough?

@nikolaykasyanov
Copy link
Contributor

nikolaykasyanov commented Oct 5, 2017

I wonder what does yapf --version print. If it mentions its name somehow we may use the output we get from validator to determine the tool. What do you think?

@robertodr
Copy link
Contributor Author

Indeed, they print their name and version:

  • yapf --version returns yapf 0.11.0
  • clang-format --version returns clang-format version 3.9.1 (tags/RELEASE_391/final)

Maybe this could even go one step further and use validator --help to get both the tool name and the options to specify lines. I'll see what I can do.

@robertodr
Copy link
Contributor Author

robertodr commented Oct 10, 2017

Coming back to this. I actually think this is the implementation that is least prone to breaking if the CLI of the validators changes. Thus, IMO, this is ready to go as-is 😉

@nikolaykasyanov
Copy link
Contributor

@robertodr I see. Could you please fix latest commit's authorship? It looks like there's my email somehow.

@nikolaykasyanov
Copy link
Contributor

Weird, it looks like a part of my latest commit is included in your commit. Maybe rebasing helps.

@robertodr
Copy link
Contributor Author

Rebasing didn't seem to help...

@nikolaykasyanov
Copy link
Contributor

nikolaykasyanov commented Oct 17, 2017

It did help to get rid of unnecessary changes in diff though. Could you try using git comment amend --reset-author?

@robertodr
Copy link
Contributor Author

robertodr commented Oct 17, 2017

Not quite sure that helped either... If you prefer, I can close and re-open this PR. It actually did help 😸 Mac OS X tests are however failing 😕

@nikolaykasyanov
Copy link
Contributor

Ugh, seems to be Travis issue. Merging. Thank you! 🎉

@nikolaykasyanov nikolaykasyanov merged commit 3756e99 into flix-tech:master Oct 18, 2017
@robertodr robertodr deleted the yapf branch November 17, 2017 18:52
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