New formatter suggestion: make keyword call use named parameters

[Guekka]

Problem

In our code, we have some keywords with 15+ parameters (not my choice...)
Positional ordering is bound to fail, and I've already fixed many related issues

If we could instead force named arguments, errors would be much more obvious

Implementation

The main difficulty is that, as far as I know, robocop does not pass context to the formatters. For this formatter to work, a list of keyword definitions in the workspace would be needed. This could be done in a first pass, before running any formatter / check

Once we have the keyword definitions, the difficulty is the same as any standard formatter

Proof of concept

With Copilot, I wrote a quick PoC. It seems to work correctly. The main issue I had was that I need it to run before SplitTooLongLine, and the only way I found is to redefine the full list of formatters

Here's the code

[bhirsz]

Robocop not having context was a design decision, but mostly temporary one. Keeping context requires a lof of additional processing which is complex. For example looking at your code I would say it would cover most cases easily. However dynamic imports, import with variables, duplicated keyword names, order of imports etc all play a role, and its even before considering Python libraries. However I always wanted to have it, just decided to focus on other priorities first. With upcoming 8.0 (with --fix) we mostly solved all of our missing bigger features so we can plan on adding context. Like you noticed we would need to do a single pass over workspace, or better investigate how robotcode does it (they scan definitions for each file, in consideration of all above restraints). It could be mode for linter and formatter, i.e. --context that will enable context aware linters and formatters.

That being said, your poc looks okay (though like I said it only covers some use cases). The limitation with order of formatters is understable - if it would be added to robocop we could add it to our ordering. However I have feeling it would work better as a rule. Rule that detects not named arguments with optional fix (now possible in 8.0) if the context is enabled and contains definition. It could be 'unsafe fix' (I replicated similar modes and behaviours from ruff fix). Rule fixes works differently than formatter since it modifies the source code using text base edits, not model base. Otherwise they work similar. The benefit is that we can use linter to catch it as issue and then decide to fix it manually. Formatter always run and always fix . I had feeling that our existing formatter should focus more on formatting and things likw replacing deprecated code or removing/adding usually should be handled by rule fix to have better control over it.

[bhirsz]

I will think about the proposal and what could be next steps. Maybe creating basic context is easier than I think and we could work towards MVP.

[Guekka]

Thanks for this detailed (and very quick) answer. I agree this is more of a lint than a formatting, but I was not aware of the upcoming --fix

By the way, I just discovered robocop and I already love it. It makes our codebase (made by non-developers) much easier to read. So thank you for the tool 🙂

[bhirsz]

Thank you! That's actually how it started for me, I was fed up with legacy project without any code guidelines - freedom is both gift and cure for Robot Framework :)

--fix will be covered in release notes and documentation (working on it for tommorrow release) but one of the new rule fixes shows it nicely:

robotframework-robocop/src/robocop/linter/rules/deprecated.py

Lines 344 to 358 in 8a04e74

	def check(self, node: Statement) -> None:
	if ROBOT_VERSION.major < 6:
	return
	setting_name = node.data_tokens[0].value.lower()
	if setting_name == "force tags":
	self.report(
	node=node, col=utils.token_col(node, Token.FORCE_TAGS), end_col=node.col_offset + len(setting_name) + 1
	)
	def fix(self, diag: Diagnostic, source_lines: list[str]) -> Fix | None: # noqa: ARG002
	return Fix(
	edits=[TextEdit.replace_at_range(self.rule_id, self.name, diag.range, "Test Tags")],
	message="Replace Force Tags with Test Tags",
	applicability=FixApplicability.SAFE,
	)

I'm working on change of design as well, 8.0 brings many changes to configuration layer or how we store our rules, focus is on full typing safety and performance (already achieved good gains with caching recently). In the above example you can see how I divided 'visiting' from checking and fixing, which is now stored in dedicated Robocop rule. The aim is to have single pass instead of multiple (we currently revisit the same file for each checker) and just 'delegate' to linked rule which should perform check and potential fix.
All the performance gains, and general design improvements are enablers for features like context aware robocop - it's a lot easier to add new code and also we don't need to worry too much about impacting performance.

[Guekka]

Sounds good! Indeed, this new architecture seems much better for performance, and it looks like it's arriving right in time for this context use case 😛