search

snazzy-d / sdc

The Snazzy D Compiler
MIT License
246 stars 55 forks source link

sdfmt: Support stdin input and `--assume-filename` option #363

Open kinke opened 4 months ago

kinke commented 4 months ago

Implementing #262.

The only thing I've tested is this: echo "module abc;" | bin/sdfmt -, which did output the expected module abc; (extra space removed) to stdout. ;)

deadalnix commented 4 months ago

Hey, thanks for the PR. Thanks to github, I did not see that earlier, as github provide no useful dashboard of what one has to review.

Anyways, a couple of remarks.

  1. There need to be some kind of test that ensure the behavior remains as expected.
  2. I'm not too sure about the assume filename feature. What is it for?
  3. Can't be just open stdin as any other file and let the magic occur? Why do we need so much special casing for stdin?
  4. Why not simply go for "If no file is provided, read on stdin" behavior? That seems to me like the most straightforward.
JohanEngelen commented 4 months ago

this functionality helps with editor support. The editor can send stuff through stdin (an edited but not saved file, for example), while letting sdfmt do config lookup as-if it is processing the file specified by --assume-filename. This functionality is used by sublimetext plugin for clang-format

kinke commented 3 months ago

this functionality helps with editor support. The editor can send stuff through stdin (an edited but not saved file, for example), while letting sdfmt do config lookup as-if it is processing the file specified by --assume-filename.

Yeah, we at Symmetry need it for exactly the same reason, for our own little VS Code extension formatting the SIL .d files (just giving Amaury the context). [It currently needs to dump stdin to a temp file in the original source dir, which updates the directory modification timestamp, which is detrimental for our reggae/ninja build based on timestamps).

  1. Can't be just open stdin as any other file and let the magic occur? Why do we need so much special casing for stdin?

Not AFAICT, I was looking for some readAll[Text]() function or so taking a File, not a path string, but haven't found anything in Phobos after a quick search. So the chunkBy(N).join was the simplest I could come up with. The rest (convertToUTF8()) is just taken from the other registerFile() overload for a file on disk.

  1. Why not simply go for "If no file is provided, read on stdin" behavior? That seems to me like the most straightforward.

Fine by me, no preference from my side (DMD/LDC use -, that's all).

  1. There need to be some kind of test that ensure the behavior remains as expected.

Would you have any handy pointers?

kinke commented 3 months ago

Okay, I've changed it to read from stdin if no files are provided in the command line, and fixed the config file lookup (fake path set too late...).

Wrt. tests, I've had a quick glance and decided to do a 2nd pass for the current sdfmt tests - passing all of these files via stdin in the 2nd pass, and using the --assume-filename option to pass the original path, which should result in exactly the same behavior as formatting the file from disk directly (the 1st pass).

deadalnix commented 3 months ago

I've been reading this multiple times, and I'm still not sure how to be directive about it, but it seems clear to me that there is a problem in approach. We are adding special cases all over the place, and the special case path often has more code than the happy case.

kinke commented 3 months ago

'All over the place' = the 2 tiny ifs in the sdfmt driver?

As you haven't given me any pointers for your custom test framework, I came up with something. Please do feel free to rework that.