v icon indicating copy to clipboard operation
v copied to clipboard

fmt: a regression was introduced in #22025 (vfmt struct init)

Open larpon opened this issue 1 year ago • 3 comments

Describe the bug

I'm not sure if the follwing vfmt struct init align changes in #22025 was intentional, but in vab a one-liner field: if {} else {} was "unrolled" resulting in a situation where I had to run v fmt -w . twice (See this diff https://github.com/vlang/vab/pull/287/files#diff-d7aec8d71af6b80eee9d9768f16fd94d842270b868b008bf268679456c13df84R520-R527)

The code in question went from:

	deploy_opt := android.DeployOptions{
		verbosity: opt.verbosity
		format: format
		// keystore: keystore
		activity_name: opt.activity_name
		work_dir: opt.work_dir
		v_flags: opt.v_flags
		device_id: opt.device_id
		deploy_file: opt.output
		kill_adb: os.getenv('VAB_KILL_ADB') != ''
		clear_device_log: opt.clear_device_log
		device_log: opt.device_log || opt.device_log_raw
		log_mode: if opt.device_log_raw { android.LogMode.raw } else { android.LogMode.filtered }
		log_tags: log_tags
		run: run
	}

to:

	deploy_opt := android.DeployOptions{
		verbosity: opt.verbosity
		format:    format
		// keystore: keystore
		activity_name:    opt.activity_name
		work_dir:         opt.work_dir
		v_flags:          opt.v_flags
		device_id:        opt.device_id
		deploy_file:      opt.output
		kill_adb:         os.getenv('VAB_KILL_ADB') != ''
		clear_device_log: opt.clear_device_log
		device_log:       opt.device_log || opt.device_log_raw
		log_mode:         if opt.device_log_raw {
			android.LogMode.raw
		} else {
			android.LogMode.filtered
		}
		log_tags: log_tags
		run:      run
	}

Reproduction Steps

It can be seen locally by checking out vlang/vab on master and run v fmt -w cli/options.v

Expected Behavior

I'd argue that the output is prettier if the if {} else {} was kept on one line. Alternatively if it should be "unrolled" it should not take two runs of v fmt -w ..

	deploy_opt := android.DeployOptions{
		verbosity: opt.verbosity
		format:    format
		// keystore: keystore
		activity_name:    opt.activity_name
		work_dir:         opt.work_dir
		v_flags:          opt.v_flags
		device_id:        opt.device_id
		deploy_file:      opt.output
		kill_adb:         os.getenv('VAB_KILL_ADB') != ''
		clear_device_log: opt.clear_device_log
		device_log:       opt.device_log || opt.device_log_raw
		log_mode:         if opt.device_log_raw { android.LogMode.raw } else { android.LogMode.filtered }
		log_tags:         log_tags
		run:              run
	}

Current Behavior

	deploy_opt := android.DeployOptions{
		verbosity: opt.verbosity
		format:    format
		// keystore: keystore
		activity_name:    opt.activity_name
		work_dir:         opt.work_dir
		v_flags:          opt.v_flags
		device_id:        opt.device_id
		deploy_file:      opt.output
		kill_adb:         os.getenv('VAB_KILL_ADB') != ''
		clear_device_log: opt.clear_device_log
		device_log:       opt.device_log || opt.device_log_raw
		log_mode:         if opt.device_log_raw {
			android.LogMode.raw
		} else {
			android.LogMode.filtered
		}
		log_tags: log_tags
		run:      run
	}

Possible Solution

No response

Additional Information/Context

No response

V version

V 0.4.7 c51d30b

Environment details (OS name and version, etc.)

Linux

[!NOTE] You can use the 👍 reaction to increase the issue's priority for developers.

Please note that only the 👍 reaction to the issue itself counts as a vote. Other reactions and those to comments will not be taken into account.

larpon avatar Aug 11 '24 09:08 larpon

At the least, it would've looked "better" (subjectively, of course) if it had been formatted as

deploy_opt := android.DeployOptions{
		verbosity: opt.verbosity
		format:    format
		// keystore: keystore
		activity_name:    opt.activity_name
		work_dir:         opt.work_dir
		v_flags:          opt.v_flags
		device_id:        opt.device_id
		deploy_file:      opt.output
		kill_adb:         os.getenv('VAB_KILL_ADB') != ''
		clear_device_log: opt.clear_device_log
		device_log:       opt.device_log || opt.device_log_raw
		log_mode:         if opt.device_log_raw {
					android.LogMode.raw
				  } else {
					android.LogMode.filtered
				  }
		log_tags: log_tags
		run:      run
	}

In other words, choose the right-hand alignment point as the base for everything on the right side.

JalonSolov avatar Aug 11 '24 15:08 JalonSolov

Agreed. No matter the style, it is a bug that vfmt needs to be run twice 🙂 (first run creates incorrect formatted code)

larpon avatar Aug 11 '24 15:08 larpon

Yes, ideally, running vfmt should produce stable output, no matter what the source is.

spytheman avatar Aug 11 '24 23:08 spytheman