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

opa fmt ... displaces comments #1549

Closed
mikol opened this issue Jul 8, 2019 · 3 comments
Closed

opa fmt ... displaces comments #1549

mikol opened this issue Jul 8, 2019 · 3 comments

Comments

@mikol
Copy link
Contributor

mikol commented Jul 8, 2019

Expected Behavior

How exactly the comments are preserved is more art than science at this point. That said, the following seem like good guidelines:

A. Comments within a block should remain within the block (not appear outside of the block)
B. Block-starting comments (e.g., { # Begin) should be preserved as-is
C. Block-ending comments (e.g., } # End) should be preserved as-is

There will inevitably be cases that can’t feasibly be accommodated because we’re talking about re-commenting code after it has been formatted, which may leave the formatted code in such a state that the original comments cannot be placed in any way that match the author’s original intent.

Given the following input, I expect formatted code to preserve comments more-or-less as suggested above.

# ------------------------------------------------------------------------------
# Header

package comments

rule {
  object.b == empty
} # End rule

# Middle

fn(x) = 1 { # Else 0
  # A
  data.foo # B
  # C
} else = 2 { # Else 1
  # D
  data.bar # E
  # F
} else = 3 { # Else 2
  # G
  data.biz # H
  # I
} # End else

array = [ # M
 2, 3, 5, 7
 # N
] # End array

# Footer
# ------------------------------------------------------------------------------

Actual Behavior

# ------------------------------------------------------------------------------
# Header

package comments

rule {
	object.b == empty
}

# End rule

# Middle

fn(x) = 1 { # Else 0
	# A
	data.foo # B
	# C
} # Else 1

else = 2 {
	# D
	data.bar # E
}

# F
else = 3 { # Else 2
	# G
	data.biz # H
}

# I
# End else

array = [2, 3, 5, 7] # M

# N
# End array
# Footer
# ------------------------------------------------------------------------------

Consider:

rule {
	object.b == empty
}

# End rule

I would have expected # End rule to remain at the end of the closing curly brace as } # End rule.

Then:

fn(x) = 1 { # Else 0
	# A
	data.foo # B
	# C
} # Else 1

else = 2 {
	# D
	data.bar # E
}

# F
else = 3 { # Else 2
	# G
	data.biz # H
}

# I
# End else

The first rule body is formatted as I would expect. Each else isn’t as expected. For example. I would expect # Else 1 to appear after the else block’s opening curly braces (same as # Else 0 and # Else 2). There’s some ambiguity, however, since I (personally) would have expected the else rule heads to appear on the same line as the previous closing curly (i.e., } else = 2 { # Else 1), but that might be a subjective style consideration.

#F and #I should appear within the open and closing curly braces of the else bodies.

As for:

array = [2, 3, 5, 7] # M

# N
# End array

It’s unclear what to expect in this case since the formatting rule for the array/set rewrites the original to be 100% inline.

Steps to Reproduce the Problem

Run opa fmt ... on the input.

@tsandall
Copy link
Member

Thanks for filing this @mikol. I think it should be possible to at least make the formatting around else more consistent. For example:

  • # Else 0 and # Else 2 are in the right position but # Else 1 is not.
  • # C is in the right position but # F and # I are not.

If the fix requires the position of the closing brace characters it might be more problematic since we don't have end positions.

@stale
Copy link

stale bot commented Jan 2, 2022

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

@stale stale bot added the inactive label Jan 2, 2022
@anderseknert
Copy link
Member

Closing in favor of #4508, where we'll try to compile all ideas for the next iteration of opa fmt (and where this is included).

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

No branches or pull requests

3 participants