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

Add line and column numbers to error messages in ktlint. #251

Merged
merged 3 commits into from
Jun 1, 2018

Conversation

runningcode
Copy link
Contributor

@runningcode runningcode commented May 29, 2018

Fixes #249

Add to release notes

  • a summary of the change

I wasn't able to run tests locally, but before I spend time fixing the tests, I was wondering if this looks good.

@nedtwigg
Copy link
Member

LGTM. What error do you get when running tests locally?

@JLLeitschuh
Copy link
Member

@shyiko Would you be so kind as to review this as well?

@shyiko
Copy link

shyiko commented May 30, 2018

I'd change error message to something simpler (e.g. Error on line: %d col %d\n%s -> %d:%d: %s) (or throw something like AssertionWithLineColumnError so that spotless could decide how to best display/hide location info based on cli arguments/configuration) but it's up to you guys.

@runningcode
Copy link
Contributor Author

@nedtwigg when running locally i get
java.lang.IllegalArgumentException: No such resource kotlin/licenseheader/KotlinCodeWithoutHeader.test

seems like the resources needed to run the test aren't being picked up from dependencies? i see testlib as a test dependency though.

@nedtwigg
Copy link
Member

Spotless chains a bunch of Function<String, String> together. If the first step adds or removes a bunch of header lines, then the second step throws an error, the line numbers in the error are going to be off. Because of this, the spotless project doesn't have an opinion for how line numbers should be formatted, and I don't think we should add API for an exceptions with built-in line numbers. Spotless is focused on the kinds of formatting errors that the formatter can fix itself, because those can be composed.

Feel free to format the line numbers as-is or according to @shyiko's advice, doesn't matter which way to us. I just did ./gradlew check for master on a mac and win 10 box, both succeeded. Not sure what might be wrong on your box, @runningcode, maybe try a clean?

@runningcode
Copy link
Contributor Author

Ok, I'll format it as-is. We can improve on it later when the need arises.

I guess my IDE can't find those resources files, but the tests work fine from the command line. Updated the tests!

@nedtwigg
Copy link
Member

Can you add a link to this PR into the changelog? That helps people review what happened, and helps them see how to make similar changes in the future.

@nedtwigg nedtwigg merged commit 841ee92 into diffplug:master Jun 1, 2018
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.

SpotlessKotlin error output is missing line numbers
4 participants