Skip to content

Conversation

@systemlogic
Copy link

Not in all cases formatting of build data / code is required. Code can be written in a way which looks beautiful and easy to spot problem when maintained in tabular format using certain spacing between columns. Current implementation of buildfier results in loosing well formatted code( manually maintained) which has been needed in tabular code.

Not in all cases formatting of build data / code is required. Code can be written in a way which looks beautiful and easy to spot problem when maintained in tabular format using certain spacing between columns. Current implementation of buildfier results in loosing well formatted code( manually maintained) which has been needed in tabular code.
@aiuto
Copy link

aiuto commented Nov 10, 2023

This seems to change the whitespace of nearly every line. Did your editor save tabs as spaces?

@systemlogic
Copy link
Author

systemlogic commented Nov 14, 2023

This seems to change the whitespace of nearly every line. Did your editor save tabs as spaces?

No such settings in my vim editor, which saves tabs as spaces.

However our own internal parser works with precision of spaces and all such tabular code need to be separated with spaces. In other words, no formatting from buildifier.

@systemlogic
Copy link
Author

@aiuto ping for review request.

@mzeren-vmw
Copy link

FWIW, we are using an internal version of #985. That patch needs to be rebased and reworked. Importantly for us it supports sorting by a particular column.

@systemlogic
Copy link
Author

@pmbethe09 @larsrc-google @vladmos @aiuto

I am curious to know if anything is pending?

@Wyverald Wyverald requested a review from oreflow as a code owner September 29, 2025 22:52
@oreflow
Copy link
Collaborator

oreflow commented Dec 30, 2025

/gemini review

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a noformat directive to skip formatting for specific blocks of code, which is particularly useful for manually aligned tabular data. The implementation adds a new noFormat state to the printer and uses an advance function to preserve original spacing. While the feature is a valuable addition, I've found a few issues in the implementation. There's a significant bug in the handling of assignment expressions (AssignExpr) where the noformat logic is currently unreachable. Additionally, the logic for sequences (seq) seems to incorrectly disable noformat for multi-line lists, which contradicts the feature's purpose. I've also suggested a small improvement to the advance function to better preserve blank lines. Please see the detailed comments for suggestions.

Comment on lines +798 to 804
if p.noFormat > 0 && false {
_, from := v.LHS.Span()
p.advance(from, v.OpPos)
p.printf("%s", v.Op)
start, _ := v.RHS.Span()
p.advance(from, start)
} else {

Choose a reason for hiding this comment

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

high

The noformat logic for AssignExpr has two issues:

  1. The condition if p.noFormat > 0 && false will always be false, making the code unreachable. The && false should be removed.
  2. Inside the block, p.advance(from, start) is incorrect. from is the end position of the left-hand side, but the printer cursor is already past the operator. The advance should be from the operator's position to the start of the right-hand side. It should be p.advance(v.OpPos, start).
Suggested change
if p.noFormat > 0 && false {
_, from := v.LHS.Span()
p.advance(from, v.OpPos)
p.printf("%s", v.Op)
start, _ := v.RHS.Span()
p.advance(from, start)
} else {
if p.noFormat > 0 {
_, from := v.LHS.Span()
p.advance(from, v.OpPos)
p.printf("%s", v.Op)
start, _ := v.RHS.Span()
p.advance(v.OpPos, start)
} else {

Comment on lines +1095 to +1105
if p.noFormat > 0 {
noFormat = true
line := (*start).Line
for _, x := range *list {
xstart, _ := x.Span()
if xstart.Line != line {
noFormat = false
break
}
}
}

Choose a reason for hiding this comment

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

high

The logic to enable noformat mode for sequences appears to be restricted to single-line sequences only. The loop checks if all elements are on the same line as the opening bracket, and disables noformat if they are not. This seems to contradict the feature's goal of preserving formatting for multi-line tabular data, as seen in the new test case. To support multi-line tables, this check should probably be removed.

if p.noFormat > 0 {
		noFormat = true
	}

Comment on lines +91 to +93
if from.Line < to.Line {
p.printf("\n")
}

Choose a reason for hiding this comment

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

medium

The current implementation only inserts a single newline, which collapses multiple blank lines from the original source. To better preserve formatting within noformat blocks, consider preserving all newlines between tokens.

Suggested change
if from.Line < to.Line {
p.printf("\n")
}
for i := from.Line; i < to.Line; i++ {
p.printf("\n")
}

@tonyaiuto
Copy link

Is there anyone in the bazelbuild org willing to move this forward.
cc/ @meteorcloudy

@meteorcloudy
Copy link
Member

@vladmos @oreflow are now owning this repo, so they'll need to decide

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.

6 participants