fmt: fix alignment of struct fields init

[yuyi98]

This PR fix alignment of struct fields init.

• This is already included in the existing fmt keep tests.

// parse flags to FlagOptions struct
fn parse_flags(mut fp flag.FlagParser) FlagOptions {
	return FlagOptions{
		serve   : fp.bool('serve', 0, false, 'run in webhook server mode')
		work_dir: fp.string('work-dir', 0, work_dir, 'gen_vc working directory')
		purge   : fp.bool('purge', 0, false, 'force purge the local repositories')
		port    : fp.int('port', 0, server_port, 'port for web server to listen on')
		log_to  : fp.string('log-to', 0, log_to, "log to is 'file' or 'terminal'")
		log_file: fp.string('log-file', 0, log_file, "log file to use when log-to is 'file'")
		dry_run : fp.bool('dry-run', 0, dry_run, 'when specified dont push anything to remote repo')
		force   : fp.bool('force', 0, false, 'force update even if already up to date')
	}
}

[spytheman]

In my personal opinion, that does not seem like a clear improvement 🤔 .

Previously, I could just rg 'table: ' for example, and quickly see where all table field initializations are.

With the new changes, that would not work, since there could be an arbitrary amount of spaces after the name, so the search regexp would have to become more complex.

Visually, it does help to align unrelated expressions, but that to me, makes it a bit harder to read, especially, when there is a long field name somewhere, that will force all the other field expressions to shift farther right. The reader will then have to scan a lot of white space between the field name and the expression for it, in a list, which to me will increase the chance of reading mistakes.

[yuyi98]

Is this style OK?

return Args{
	bytes:                bytes_arg
	follow:               if f_arg { 'descriptor' } else { follow_arg }
	lines:                lines_arg
	max_unchanged_stats:  max_unchanged_stats_arg
 }

[spytheman]

Another reason to dislike this, for me, is that it increases the chances of bigger diffs in remote places, for unrelated changes, just due to formatting.

See for example https://github.com/vlang/v/pull/22000/files#diff-053e81e58870190b51e3f9c9bd0271125f412ace9be8798baa0ae745db4036f2L157-L167

In that diff, the longest field is named window_title. If in a future version of gg, it is shortened to just title, then that will cause a reduction in the whitespaces for all the other unrelated fields, in all callers to gg.new_context() too -> bigger diffs, with more chances to introduce conflicts. Note, that unlike declarations, that are localized/always single, callsites of functions, and struct initialisations are many, and distributed.

In comparison, with the current fmt version on master, changing that field in the same way, will only require local changes to the lines, that do mention the old name, without affecting the rest -> much smaller diffs, that are easier to read, with minimal chances for conflicts.

[spytheman]

Is this style OK?

No, the position of the : does not matter much to me. What matters (and what I dislike) is the variable amount of whitespace for the shorter fields, in order for the initialisations in the callsites to get aligned.

[spytheman]

@medvednikov what do you think?

Do you prefer for fmt to produce struct initializations, with aligned field values or not?

[medvednikov]

Sorry for the delay.

We should mimic gofmt's behavior here.

This is ok:

return Args{
	bytes:                bytes_arg
	follow:               if f_arg { 'descriptor' } else { follow_arg }
	lines:                lines_arg
	max_unchanged_stats:  max_unchanged_stats_arg
 }

Except there needs to be only one space after :, not two.

[medvednikov]

Also gofmt doesn't align the values if there's a newline or a comment.

[spytheman]

Can we close this, since #22025 was merged?