Add config option generated_file_header_size

[captbaritone]

Fixes #5658

FYI @ytmimi

[captbaritone]

I wonder if it would be preferable to make format_generated_files a richer config option which the line number being a sub-option that can only be configured when the rule is enabled. As it stands, generated_file_header_size does not really make sense if format_generated_files is not enabled.

[ytmimi]

@captbaritone thanks for your first contribution to rustfmt 🎉

The tests are failing because we're missing an entry in Configurations.md for the new config option. additionally it would be nice to include some tests to validate the behavior of the new option.

I wonder if it would be preferable to make format_generated_files a richer config option which the line number being a sub-option that can only be configured when the rule is enabled.

Interesting idea! Are you suggesting the configuration should be something like this:

// Default. The file is always formatted
format_generated_files = true

// Disable formatting if the `@generated` marker is found within the first 5 lines of the file.
format_generated_files = false

// Disable formatting if the `@generated` marker is found within the first n lines of the file.
format_generated_files = <usize>

[ytmimi]

Upon further consideration the format_generated_files = <usize> idea I brought up in my previous comment seems a little clunky and counter intuitive since the config is named format_generated_files and what we're trying to do here is configure the number of lines to search for the @generated marker to prevent formatting.

Let me know if you can come up with anything better than the current approach of generated_file_header_size. Personally I think it would suffice to document that the option has no impact if format_generated_files is true (the default).

[captbaritone]

Should these two config options be promoted to stable together? Or should I create a new issue to track promoting this option?

[ytmimi]

I think it's would be fine to stabilize these at the same time.

[captbaritone]

@ytmimi I've added documentation and tests.

[captbaritone]

Hey, @ytmimi just following up to see if there's any feedback on this.

[ytmimi]

Let's reword this to be clearer about when this option has an effect:

Number of lines to check for a @generated pragma header when format_generated_files is false. When format_generated_files is true, this option has no effect.

[ytmimi]

Lets add a See also [format_generated_files](#format_generated_files) link here.

[ytmimi]

Thanks for your work on this!

The implementation looks good, I'd just want us to make some minor tweaks to the documentation. Additionally can we add two more test cases:

1. Add a test case where format_generated_files=true and generated_file_header_size is set to some value where the @generated marker could be found. This would better test that when format_generated_files=true is set generated_file_header_size has no effect.
2. Add a test cast where format_generated_files=false and generated_file_header_size is set to some value where the @generated marker can't be found. This would show that even when format_generated_files=false we'll still format the file if we can't find the @generaged marker.

[calebcartwright]

Just want to add that I'm open to allowing the user to configure this value, but I haven't had a chance to consider this particular approach and I don't know if this is a significantly high enough priority for us to consider for release in the immediate future

[captbaritone]

@calebcartwright Thanks for the honesty. I'll wait for you to consider the approach more closely before I invest more time in this PR.

As it stands we are blocked from running rustftm from the CLI (needed to enforce formatting in CI) and must be careful to avoid saving generated files in our editor. Is there a workaround that you would recommend in the mean time?

[calebcartwright]

I'll wait for you to consider the approach more closely before I invest more time in this PR.

To be clear, we'll absolutely support adding an option that allows for this to be controlled, just need a chance to catch up on the back and forth within this thread as well as consider the specifics (as one initial reaction, something bothers me about the currently proposed option name as I don't think it's as self-explanatory as we'd like to see)

As it stands we are blocked from running rustftm from the CLI (needed to enforce formatting in CI) and must be careful to avoid saving generated files in our editor. Is there a workaround that you would recommend in the mean time?
Merge state

Sorry not sure what you're asking. If you want to (temporarily) block folks from running rustfmt, intentionally or accidentally, the best route would be to enable disable_all_formatting. If you want to permit formatting in general but only skip generated files, then ignore would likely be a viable option, assuming you're on a nightly toolchain

[ytmimi]

closing since we merged #5993