Ruff formatting

[svartkanin]

Related issue: #1682

I know this topic has been up a few times so I thought it's time for another round :)
Some previous discussion around using YAPF #997

As the code base has been integrated heavily with ruff it would make sense to also explore using the ruff format extension.

I run a simple ruff format archinstall/ to get a feeling of what it might look like and to collect feedback on what works and what looks funky. It seems there are some settings for the formatter but not to an extend of YAPF.

Documentation: https://docs.astral.sh/ruff/settings/#format

@Torxed any feedback would be great :)

Edit

Based on the discussions below, the general consent is to use ruff format for now as the main formatting tool.
This PR therefore

• Adds ruff formatter definition in pyproject.toml
• Formats all files
• Adds pre-commit functionality to auto-format on git commit
• Adds a git action to enforce the formatting

[svartkanin]

I'm not really a fan of things like
https://github.com/archlinux/archinstall/pull/3459/files#diff-4d8c19c70790b303e8338fbe439d7203c23bb2247a6b0c08cc995d0ca8271855L54-L58

I find the one item per line much for readable

[Torxed]

I prefer one item per line too, unless it's ~2 parameters, in which case they become few enough that I have no trouble grasping what's going on. In my brain readability is more important than uniformity (and with this that they are necessarily not the same thing).

With that said, this is absolutely a deteriorating change:

- MenuItem(
- 	text=str(_('Automatic time sync (NTP)')),
- 	action=ask_ntp,
- 	value=True,
- 	preview_action=self._prev_ntp,
- 	key='ntp'
- ),
+ MenuItem(text=str(_('Automatic time sync (NTP)')), action=ask_ntp, value=True, preview_action=self._prev_ntp, key='ntp'),

We also have to change these lines:

archinstall/CONTRIBUTING.md

Lines 32 to 34 in e61b966

	* [String quotes](https://www.python.org/dev/peps/pep-0008/#string-quotes) follow PEP8, the exception being when
	creating formatted strings, double-quoted strings are *preferred* but not required on the outer edges *(
	Example: `f"Welcome {name}"` rather than `f'Welcome {name}'`)*.

Which I'm fine with, since Python now with Python 3.13 handles inline quotation marks better within f-strings. So there's really no need to be strict about this anymore.

[svartkanin]

I was really hoping there would be more settings but it feels much like black in that sense

[correctmost]

I'm not really a fan of things like https://github.com/archlinux/archinstall/pull/3459/files#diff-4d8c19c70790b303e8338fbe439d7203c23bb2247a6b0c08cc995d0ca8271855L54-L58

I find the one item per line much for readable

This can be fixed by adding trailing commas:

ruff check --select=COM812 --fix
ruff format

The COM812 rule can then be removed from the exclusion list:

archinstall/pyproject.toml

Line 223 in e61b966

"COM812", # missing-trailing-comma

[svartkanin]

@correctmost thanks for that, I pushed an update with that and it seems to have done a bit better. But there are still some things that got moved into a single line such as

https://github.com/archlinux/archinstall/pull/3459/files#diff-ddf0aac257309838d6f80c1beb71eacc46bd77a91a006332b49096939dd18623R14

https://github.com/archlinux/archinstall/pull/3459/files#diff-4d8c19c70790b303e8338fbe439d7203c23bb2247a6b0c08cc995d0ca8271855R54

Is there something to keep those items per line as well?

[correctmost]

Oh, weird, it looks like ruff check --select=COM812 --fix didn't fix every instance because the auto-formatting already occurred on your branch.

This should work better:

# Stash any local changes
git stash

# Switch to the PR branch
git checkout ruff-format

# Restore the files to the state on master
git checkout master archinstall/ docs/ examples/ tests/

# Redo the comma fixes and auto-formatting
ruff check --select=COM812 --fix
ruff format

# Check the changes
git diff master

# --> Commit the changes and push the PR branch here

[svartkanin]

Great that worked!
I actually looks quite good now with not too many major changes.

@Torxed what do you think?

[Torxed]

It's looking better! I have a question why it's not doing the same here:

- 	part_mods = [
- 		d.segment for d in data
- 		if isinstance(d.segment, PartitionModification)
- 		and d.segment != entry
- 	]
+ 	part_mods = [d.segment for d in data if isinstance(d.segment, PartitionModification) and d.segment != entry]

[svartkanin]

I assume that's because the max line length is quite high for a python project with 160. Reducing it I can see that it starts getting broken up, e.g. with 100.

Having said that, I've noticed some other oddities such as

			prompt = (
				str(
					_(
						'This partition is currently encrypted, to format it a filesystem has to be specified'
					)
				)
				+ '\n'
			)

[Torxed]

I really wish these programs weren't so hung up on line length.
I just want the same structured format rather than it happening based on outside parameters.

But at some point, I think this PR might better benefit us, so I'll stand behind it despite the oddities if everyone else like this :)

[svartkanin]

I'd be in favour of reducing the line count though, to maybe 120, unless there's a good reason to keep it at 160?

I can try running the formatter with yapf as well to compare the output, that one has lots of options to tweak which might suit better?

[Torxed]

I just wish it could keep strings long. I prefer one slightly long string over many smaller strings over multiple lines. Preference things I guess, but try and see what 120 line length would look like. I suspect it will chop up 80% of our function definitions.

[svartkanin]

It seems that the ruff format collides with flake8

archinstall/lib/disk/device_handler.py:420:2: E704 multiple statements on one line (def)
	def _lvm_info_with_retry(self, cmd: str, info_type: Literal['lv']) -> LvmVolumeInfo | None: ...
	^
archinstall/lib/disk/device_handler.py:423:2: E704 multiple statements on one line (def)
	def _lvm_info_with_retry(self, cmd: str, info_type: Literal['vg']) -> LvmGroupInfo | None: ...
	^
archinstall/lib/disk/device_handler.py:426:2: E704 multiple statements on one line (def)
	def _lvm_info_with_retry(self, cmd: str, info_type: Literal['pvseg']) -> LvmPVInfo | None: ...
	^
archinstall/lib/networking.py:154:76: E203 whitespace before ':'
		checksum += (icmp_packet[i] << 8) + (struct.unpack('B', icmp_packet[i + 1 : i + 2])[0] if len(icmp_packet[i + 1 : i + 2]) else 0)
		                                                                        ^
archinstall/lib/networking.py:154:114: E203 whitespace before ':'
		checksum += (icmp_packet[i] << 8) + (struct.unpack('B', icmp_packet[i + 1 : i + 2])[0] if len(icmp_packet[i + 1 : i + 2]) else 0)
		                                                                                                              ^
archinstall/lib/networking.py:195:59: E203 whitespace before ':'
				if icmp_type == 0 and response[-len(random_identifier) :] == random_identifier:
				                                                     ^
archinstall/tui/menu_item.py:443:12: E203 whitespace before ':'
				items[x : x + self._total_cols],

I found some mention in an issue astral-sh/ruff#8642 (comment) which then later points to what black recommends to ignore them https://ichard26-testblackdocs.readthedocs.io/en/refactor_docs/compatible_configs.html#configuration.

I pushed another version formatted with yapf and the following settings

[tool.yapf]
based_on_style = "pep8"
use_tabs = true
column_limit = 160
align_closing_bracket_with_visual_indent = true
blank_line_before_nested_class_or_def = true
coalesce_brackets = true
dedent_closing_brackets = true
continuation_align_style = "VALIGN-RIGHT"
spaces_before_comment = 2
split_before_first_argument = true
split_before_closing_bracket = true
split_all_top_level_comma_separated_values = true
indent_dictionary_value = true
split_arguments_when_comma_terminated = false

I had to remove the type variables in classes and functions as yapf couldn't handle them. I raised an issue in yapf for it google/yapf#1277.

The formatting looks similar to ruff but it allows for more tweaks and doesn't seem to suffer from the same above flake8 deviations.

[correctmost]

It seems that the ruff format collides with flake8

If ruff is used for auto-formatting, it might be okay to remove the flake8 checks. The current Pylint + ruff + mypy checks should cover most of the flake8 checks.

I had to remove the type variables in classes and functions as yapf couldn't handle them. I raised an issue in yapf for it google/yapf#1277.

Looking at the commit log, it doesn't seem like yapf is actively developed. I don't think it would be good to rely on a formatter that won't be able to handle new Python syntax.

[svartkanin]

Agreed, I think moving forward with ruff is probably the better option for now.

[svartkanin]

@Torxed I've updated the PR with all files formatted, I left the current 160 line length for now, we can adjust it in a follow up if needed.

[Torxed]

Sounds good, and good job elaborating on this one!