Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/implement formatter #291

Merged
merged 29 commits into from
May 3, 2024
Merged

Feature/implement formatter #291

merged 29 commits into from
May 3, 2024

Conversation

ysugimoto
Copy link
Owner

@ysugimoto ysugimoto commented Apr 2, 2024

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.

Comment on lines +64 to +78
// Formatter options
IndentWidth int `yaml:"indent_width" default:"2"`
TrailingCommentWidth int `yaml:"trailing_comment_width" default:"2"`
IndentStyle string `yaml:"indent_style" default:"space"`
LineWidth int `yaml:"line_width" default:"120"`
ExplicitStringConat bool `yaml:"explicit_string_concat" default:"false"`
SortDeclarationProperty bool `yaml:"sort_declaration_property" default:"false"`
AlignDeclarationProperty bool `yaml:"align_declaration_property" default:"false"`
ElseIf bool `yaml:"else_if" default:"false"`
AlwaysNextLineElseIf bool `yaml:"always_next_line_else_if" default:"false"`
ReturnStatementParenthesis bool `yaml:"return_statement_parenthesis" default:"true"`
SortDeclaration bool `yaml:"sort_declaration" defaul:"false"`
AlignTrailingComment bool `yaml:"align_trailing_comment" default:"false"`
CommentStyle string `yaml:"comment_style" default:"none"`
ShouldUseUnset bool `yaml:"should_use_unset" default:"false"`
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are all formatting rules, see #274

@richardmarshall
Copy link
Collaborator

richardmarshall commented Apr 2, 2024

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.

@ysugimoto
Copy link
Owner Author

ysugimoto commented Apr 2, 2024

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

@richardmarshall
Copy link
Collaborator

richardmarshall commented Apr 4, 2024

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;
  }
}

@ysugimoto
Copy link
Owner Author

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.

@richardmarshall
Copy link
Collaborator

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
Copy link
Collaborator

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

@ysugimoto
Copy link
Owner Author

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

@ysugimoto
Copy link
Owner Author

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 ysugimoto merged commit 9c9bb69 into main May 3, 2024
4 checks passed
@ysugimoto ysugimoto deleted the feature/implement-formatter branch May 3, 2024 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants