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

Fix a trivial bug in error reporting in the kythe indexing task. #5913

Merged
merged 1 commit into from Jun 5, 2018

Conversation

Projects
None yet
3 participants
@benjyw
Copy link
Contributor

benjyw commented Jun 5, 2018

No description provided.

@benjyw benjyw requested a review from stuhood Jun 5, 2018

@stuhood

stuhood approved these changes Jun 5, 2018

Copy link
Member

stuhood left a comment

I won't say it. But it rhymes with "demonstration best".

@baroquebobcat
Copy link
Contributor

baroquebobcat left a comment

Looks good to me, though I'd also like an integration test.

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Jun 5, 2018

I have to disagree. Tests aren't free, and "does this one exception in this one task format its error message without failing" seems A) arbitrary and B) too trivial to be worth the cost of everyone's CIs getting costlier in perpetuity.

@benjyw benjyw merged commit f6918ff into pantsbuild:master Jun 5, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@baroquebobcat

This comment has been minimized.

Copy link
Contributor

baroquebobcat commented Jun 5, 2018

I see what you mean. I've actually been meaning to talk about coming up with an abstraction to replace the current if result != 0 blocks that have ended up in a bunch of places. I think a more general approach would make things like this less likely.

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jun 5, 2018

Ah, apologies. My bikeshed paint shopping missed the fact that this is only an error message, and not the actual selection of the entrypoint, which would have been more egregious. Totally agree.

@benjyw benjyw deleted the benjyw:fix_kythe_index_error branch Jun 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment