Skip to content

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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kinke
Copy link
Contributor

@kinke kinke commented Apr 23, 2024

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
Copy link
Contributor

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
Copy link
Contributor

JohanEngelen commented Apr 29, 2024

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

const sanitizedData = convertToUTF8(stdinData);

const fakeFilename =
assumeFilename.length ? assumeFilename : "stdin";
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't "stdin" be changed to something like "." such that config file lookup starts from cwd ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assumed that's exactly what it'd do, treating it like a local stdin.d file in cwd (but having a nicer stdin 'name' in potential error msges etc.). But totally untested. ;)

@kinke
Copy link
Contributor Author

kinke commented Apr 29, 2024

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
Copy link
Contributor Author

kinke commented May 2, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

kinke commented May 25, 2024

'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.

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.

3 participants