falco icon indicating copy to clipboard operation
falco copied to clipboard

Feature/implement formatter

Open ysugimoto opened this issue 11 months ago • 5 comments

This PR aims to provide formatter feature in falco.

Usage (command)

I added a new fmt subcommand.

## multiple files
falco fmt /path/to/defaut.vcl /path/to/module.vcl ...

## Supports glob pattern
falco fmt /path/to/**/*.vcl

falco outputs formatted result to stdout, but overwrites the input file by providing -w, --write argument.

falco fmt -w /path/to/default.vcl

Configuration

Formatting rules could be customized in format section .falco.yaml configuration file:

...
format:
  [some override configurations here]
...

Implemented formatting rules are described discussion Now I have tested with huge VCLs that are used for production, it seems fine.

And I put some comments on this PR, please take a look also.

ysugimoto avatar Apr 02 '24 17:04 ysugimoto

Haven't started looking through the implementation yet but tested this against my production VCL and it produced a lot of problems. They all seem to stem from 4 types of errors in the formatted output.

Input:

sub boolfn BOOL {
  return true;
}

sub vcl_recv {
  if (
    // foo
    req.http.foo == "bar"
    // bar
    && req.http.bar == "baz"
  ) {
    esi;
  } elseif (req.http.foo) {
    esi;
  }
}

Output:

sub boolfn { // <-- 1) return type removed
  return (true); // <-- 2) parens added around return value
}


sub vcl_recv {
  if (// foo req.http.foo == "bar"  // bar && req.http.bar == "baz") { // <-- 3) multi-line conditional with line comments collapsed
    esi;
  } ( (req.http.foo) { // <-- 4) `elseif` replaced with `(`
    esi;
  }
}

These were the issues that produced errors, will look through the output for other non-error causing formatting issues when I have some more time.

richardmarshall avatar Apr 02 '24 18:04 richardmarshall

Ah, these are some cases that I have not considered, thanks I will look and fix it.

ysugimoto avatar Apr 02 '24 19:04 ysugimoto

Got a start on looking through the implementation this morning and will take a look at the updates as well when I can.

In the meantime I noticed a few formatting issues that don't cause syntax errors like the previous examples I shared.

Input:

sub unary_prefix_operator {
  if (!req.http.foo) { esi; }
  declare local var.i INTEGER;
  set var.i = -5;
}

director unary_postfix_operator client {
  .quorum = 20 %;
}

sub switch_formatting {
  switch (req.http.foo) {
      case "1":
        esi;
        break;
  default: break; case "2": esi; break;
  }
}

Formatted output:

sub unary_prefix_operator {
  if (! req.http.foo) { <- space inserted between unary prefix operator and operand
    esi;
  }
  declare local var.i INTEGER;
  set var.i = - 5; <- space inserted
}

<- extra newline inserted between declarations, is this intended?
director unary_postfix_operator client {
  .quorum = 20%;  <- in comparison for the postfix operator no space inserted (which is what I would expect)
}


sub switch_formatting {
  switch (req.http.foo) {
    case "1": <- case indented
    esi; <- not indented relative to case statement
      break; <- indented correctly relative to case statement but too far relative to the previous line
    default:
      break;
    case "2":
    esi;
      break;
  }
}

richardmarshall avatar Apr 04 '24 02:04 richardmarshall

I found a problem in PrefixExpression, I will fix it.

My preference is that case statement should be indented but I will add Indent case labels whether indent or not as you said in Discussion.

ysugimoto avatar Apr 04 '24 07:04 ysugimoto

While doing some more looking at the results of formatting various real world VCL files I notice comments getting lost in a few places. Given how critical it is for a formatter to not lose anything I started looking in more detail at how comment attachment is currently handled and all the places comments can be placed.

This led to what I wrote up in #299 and the test PR #300.

richardmarshall avatar Apr 07 '24 06:04 richardmarshall

I hope to have some free time this weekend to get back to reviewing this.

richardmarshall avatar Apr 18 '24 14:04 richardmarshall

@richardmarshall Could you take a look #302 before? This PR improves comment dealing with more complexity.

ysugimoto avatar Apr 19 '24 11:04 ysugimoto

This PR could be merged by fixing some comment issues. I will merge this in a few days so please make another PRs if you find any problems.

ysugimoto avatar Apr 30 '24 10:04 ysugimoto