Add flag to check if files are formatted

[ratsclub]

This is a follow-up PR for #91.

About the flag name, what do you think of --dryrun?

[ratsclub]

Another issue I'm facing is that the pretty output doesn't have a break line at the end of the file. Do you think we should trim the original one in order to compare?

[shwestrick]

Another issue I'm facing is that the pretty output doesn't have a break line at the end of the file. Do you think we should trim the original one in order to compare?

I think the formatted output should always be exactly result ^ "\n" -- does that work?

[shwestrick]

About the flag name, what do you think of --dryrun?

I like it, although I think the name "dry run" suggests that it will tell us exactly what changes a normal run would make. (For example, git clean --dry-run.)

This wouldn't be a huge change; it would just require not immediately failing on the first file, and instead allowing the tool to continue.

The output could be something like:

Would modify path/to/foo.sml
Would modify bar.sml

Thoughts?

[ratsclub]

@shwestrick Would you be open to give this the --check flag? Other tools that use it:

• black
• rustfmt
• fantomas

In this case, I suggest changing the current --check to --safety-check.

[shwestrick]

@shwestrick Would you be open to give this the --check flag? Other tools that use it:

• black
• rustfmt
• fantomas

Oh excellent, I like it. Let's use --check then.

In this case, I suggest changing the current --check to --safety-check.

Sounds good to me!

[ratsclub]

@shwestrick May you check if this is good enough, please?

[shwestrick]

Just did a little testing and found a bug. (This file should pass.)

$ ./smlfmt --check src/base/MLtonPathMap.sml
ERROR: Unformatted file 'src/base/MLtonPathMap.sml'

The issue appears to be on this line:

val original = TextIO.input (TextIO.openIn hfp)

This is subtle, but TextIO.input doesn't guarantee that you get all the input.

If I switch to this instead, it seems to work:

val original = ReadFile.contents hfp

(ReadFile.contents is usually faster, gets the entire contents, and also guarantees that the opened file is closed before returning.)

Could you make that change? Then I think we're good to merge.

[ratsclub]

This is subtle, but TextIO.input doesn't guarantee that you get all the input.

This is surprising and good to know. Do you have any place where I can read more about that?

Could you make that change? Then I think we're good to merge.

Done.

[shwestrick]

The official docs for TextIO.input are a bit vague but technically allow for this behavior: "... returns a vector of at least one element"

Moscow ML's description here is perhaps at little better: "... returns some elements from [the stream]"

My understanding is that this function has a very particular intended mode-of-use, where you call TextIO.input(...) in a loop until it returns an empty string. This would let you incrementally process characters from a large file without needing the entire string in memory all at once. (There is a separate function TextIO.inputAll which gets you the entire string.)

The naming is unfortunate (the name input is unassuming but the function is very niche), and I wish the docs were more clear. Perhaps it all made a bit more sense back in the 80s and 90s when the Basis Library was designed. In 2023, I find it a bit strange. Oh well 🤷‍♂️

[shwestrick]

Thanks for putting this together, it looks great! Merging now.