📎 Deprecate the CLI option --indent-size and create --indent-width

[ematipico]

Description

• create a new --indent-width CLI option;
• log a deprecation diagnostic if --indent-size is provided;
• log a warning diagnostic if --indent-size and --indent-width are provided, and use the latter;
• include tests

[Levieber]

I will try to do this

[Levieber]

Should I use/create a diagnostic struct for this? Where should I check the provided option?

I'm thinking of doing based on #107 if you have any tips better.

[ematipico]

Should I use/create a diagnostic struct for this? Where should I check the provided option?

Yes, you'll have to create a new diagnostic.

biome/crates/rome_cli/src/diagnostics.rs

Lines 23 to 60 in ef3af45

	pub enum CliDiagnostic {
	/// Returned when it is called with a subcommand it doesn't know
	UnknownCommand(UnknownCommand),
	/// Return by the help command when it is called with a subcommand it doesn't know
	UnknownCommandHelp(UnknownCommandHelp),
	/// Returned when the value of a command line argument could not be parsed
	ParseError(ParseDiagnostic),
	/// Returned when the CLI doesn't recognize a command line argument
	UnexpectedArgument(UnexpectedArgument),
	/// Returned when a required argument is not present in the command line
	MissingArgument(MissingArgument),
	/// Returned when a subcommand is called without any arguments
	EmptyArguments(EmptyArguments),
	/// Returned when a subcommand is called with an unsupported combination of arguments
	IncompatibleArguments(IncompatibleArguments),
	/// Returned by a traversal command when error diagnostics were emitted
	CheckError(CheckError),
	/// Emitted when a file is fixed, but it still contains diagnostics.
	///
	/// This happens when these diagnostics come from rules that don't have a code action.
	FileCheck(FileCheck),
	/// When an argument is higher than the expected maximum
	OverflowNumberArgument(OverflowNumberArgument),
	/// Wrapper for an underlying `rome_service` error
	WorkspaceError(WorkspaceError),
	/// Wrapper for an underlying `std::io` error
	IoError(IoDiagnostic),
	/// The daemon is not running
	ServerNotRunning(ServerNotRunning),
	/// The end configuration (`biome.json` + other options) is incompatible with the command
	IncompatibleEndConfiguration(IncompatibleEndConfiguration),
	/// No files processed during the file system traversal
	NoFilesWereProcessed(NoFilesWereProcessed),
	/// Errors thrown when running the `biome migrate` command
	MigrateError(MigrationDiagnostic),
	/// When the VCS folder couldn't be found
	NoVcsFolderFound(NoVcsFolderFound),
	}

You could create a generic DeprecatedArgument inside this enum, that accepts a generic message. Or a loose diagnostic like in #107. This the latter is a better solution

I'm thinking of doing based on #107 if you have any tips better.

Yes that's right!

[Levieber]

Should I create an array to store all the deprecated arguments, in order to leave it generic for future uses?

[ematipico]

Should I create an array to store all the deprecated arguments, in order to leave it generic for future uses?

It's possible that in the future we don't deprecate an option for another, that's why a generic message is more future proof

[Levieber]

When I run the CLI command (cargo run --bin biome -- --help), it gives the following error: thread 'main' has overflowed its stack.

I'm using Windows.

[ematipico]

To run the CLI, do

cargo biome-cli-dev --help

[Levieber]

I tested this command now, and it also gave the same error.

[ematipico]

Oh, I guess the previous windows users didn't use this command 😔 not sure how to fix that, I don't use windows

You can still write a test, they are very reliable, and we have many of them

[Levieber]

I'm sorry I didn't ask for this information sooner

What would be the best or correct way to add this option? I tried putting it in the formatter configuration in the rome_service crate, but that breaks tests that apparently don't use the indent-size option through the CLI.

I checked the deprecated option in the or_diagnostic function equal to #107.

Is there anything I forgot to do or did wrong?

[ematipico]

What would be the best or correct way to add this option? I tried putting it in the formatter configuration in the rome_service crate, but that breaks tests that apparently don't use the indent-size option through the CLI.

That's the correct place. I'm not sure what you mean when tests break, maybe the best option is opening a draft PR so we can look a it, when code at hand.

[Levieber]

Explaining better: the way I used to check the indent-size option (very similar to #107) apparently takes the settings from the configuration file, which causes the diagnostic to be run even without pass the deprecated CLI option.

If you still don't understand, I committed an early version (without tests) of what I tried to do, and I can make a draft pull request.

[ematipico]

I understand now. Still, it would be great to see some code, so I can better understand the implementation, and give suggestions. It's fine there aren't any tests, that's what draft PRs are also good for: get feedback

[Conaclos]

I first thought this was a good idea. However, I am starting to wonder if it is still. Prettier and .editorConfig use indent_size.

[ematipico]

width is more in line with the meaning of the word given its context, because it's applied on a line. And it's also in line with line_width.