Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Fixes #61: Catch yapf errors instead of overwriting file #62

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

Merged
dbarnett merged 3 commits into google:master from achalddave:master
Nov 7, 2015

Conversation

@achalddave
Copy link
Contributor

@achalddave achalddave commented Nov 2, 2015

This isn't perfect, since it doesn't try to parse the yapf output, but it at least won't overwrite the file if there's an error.

Copy link
Collaborator

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

Copy link
Contributor Author

Just signed the CLA.

Copy link
Collaborator

CLAs look good, thanks!

Copy link
Contributor

@dbarnett dbarnett Nov 2, 2015

Choose a reason for hiding this comment

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

Could you drop this reassignment while you're in here? AFAICT it serves no purpose.

Copy link
Contributor Author

@achalddave achalddave Nov 2, 2015

Choose a reason for hiding this comment

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

Oops, didn't notice that. Done.

Copy link
Contributor Author

This is actually wrong, and I don't think I'm familiar enough with vimscript to fix it. Specifically, yapf returns error code 2 if the error is changed, which means I have to catch the ShellError exception, then check if the error code is 2, and do the same formatting logic as in the try. Something like

 try
 let l:result = maktaba#syscall#Create(l:cmd).WithStdin(l:input).Call()
 catch /ERROR(ShellError):/
 if v:shell_error == -1
 call maktaba#error#Shout('Error formatting file: %s', v:exception)
 endif
 endtry
 let l:formatted = split(l:result.stdout, "\n")
 call maktaba#buffer#Overwrite(1, line('$'), l:formatted)

doesn't work since l:result is scoped to within the try block... I don't know how to declare result to have scope outside the try block in vimscript.

Update: for now, I just got rid of the try catch and checked the v:shell_error variable manually.

yapf only returns an error code of 0 if it does not modify the file.
If it does modify the file, it returns error code 2, which causes
maktaba to throw a ShellError. Now, we just manually check the error
code instead of attempting to catch the exception.
Copy link
Contributor Author

Does this look good? I've been using it for the past few days and it seems to work fine.

Copy link
Contributor

dbarnett commented Nov 7, 2015

Yep, looks good. Thanks!

dbarnett added a commit that referenced this pull request Nov 7, 2015
Fixes #61: Catch yapf errors instead of overwriting file
@dbarnett dbarnett merged commit 356212d into google:master Nov 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /