Make ptimports work on Windows

[dlwyatt]

and support files that contain CRLF as their EOL sequence.

[nmiyake]

Thanks for the PR!

Most of the core ptimports code is based on goimports (https://github.com/golang/tools/blob/master/cmd/goimports/goimports.go). Is this an issue there as well, or does this issue only arise because ptimports adds newlines?

[dlwyatt]

No idea! The behavior with backslashes versus slashes was the real problem in ptimports. I just did the CRLF thing to make git stop whining at me about changing line endings. :)

[dlwyatt]

Looks like yes, goimports also inserts plain newlines:

C:\Users\dwyatt\Documents\Git\go\src\github.com\palantir\checks\ptimports [windows-support ≡]
λ  goimports -w .\ptimports.go
C:\Users\dwyatt\Documents\Git\go\src\github.com\palantir\checks\ptimports [windows-support ≡ +0 ~1 -0 !]
λ  git diff
warning: LF will be replaced by CRLF in ptimports/ptimports.go.
The file will have its original line endings in your working directory.
C:\Users\dwyatt\Documents\Git\go\src\github.com\palantir\checks\ptimports [windows-support ≡ +0 ~1 -0 !]

Never noticed that before because I use VSCode, and apparently it takes care of that already when it runs goimports.

[dlwyatt]

I submitted a similar PR for goimports to handle the crlf line endings: https://go-review.googlesource.com/c/tools/+/83535

[dlwyatt]

I've updated the PR to align with the changes they won't make to goimports / gofmt. The rest of the windows fixes are still in place, and it will still treat files as correct if the only difference is line endings.

[nmiyake]

There seem to be a few different things going on here. Here's my overview:

• If ptimports doesn't run properly on Windows due to path or separator issues, that's definitely a bug. If this is the case, please file an issue and then open a PR that fixes just that bug.
• ptimport inserts newlines in some places. The behavior of what kind of newlines are inserted (\n versus CRLF) should match gofmt and ptimports for consistency. It seems that those tools only insert newlines (\n), so unless/until that changes, the behavior here should be consistent.
  • Beyond just being consistent, it's important that the output of ptimports is not modified if run through gofmt or goimports.
• With regards to ignoring newlines for diffs -- I think another important invariant is that, if ptimports (or gofmt or whatever else) would modify a file if it were run on it, then it should appear in the -l output. I haven't tested this, but if running a CRLF file through these files would modify it, then I think it is correct for the file to appear in the -l output

Each of these top-level bullet points are independent concerns, so would like to address them in separate PRs/issues as well.

Finally, as a heads-up, for the purposes of gödel all of this may be less of a concern, since for 2.0 I'm strongly considering not using ptimports by default (may switch to just gofmt or goimports). Will link to the issue in that repo once I create it.

[nmiyake]

gödel issue referenced above is at palantir/godel#269.

[nmiyake]

Thanks, content looks good! Underlying code has changed since your initial commit and conflicts now exist -- could you update this PR to fix the conflicts? Once that is done, this should be good to merge.

[dlwyatt]

Done