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 request: inline functions with if statements #76

Open
ntjess opened this issue May 25, 2024 · 11 comments · May be fixed by #77
Open

Feature request: inline functions with if statements #76

ntjess opened this issue May 25, 2024 · 11 comments · May be fixed by #77
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@ntjess
Copy link

ntjess commented May 25, 2024

Consider this case:

#table(
  fill: (x, y) => if y == 0 { white.darken(15%) } else { none },
  align: (x, y) => if y == 0 { center } else { horizon },
  [Hi],
  [there],
)

The function definition is short (< line length) and clearly indicates parameter usage. typstyle reformats to:

#table(
  fill: (x, y) => if y == 0 {
    white.darken(15%)
  } else {
    none
  },
  align: (x, y) => if y == 0 {
    center
  } else {
    horizon
  },
  [Hi],
  [there],
)

which is arguably less readable.

My request is for inline ternary functions shorter than the user-specified line length to remain one-line.

@Enter-tainer
Copy link
Owner

Enter-tainer commented May 26, 2024

it is related to how we format code blocks. currently code blocks are always take at least 3 lines. i think we can make a special case for this.

@Enter-tainer Enter-tainer added the enhancement New feature or request label May 26, 2024
@Enter-tainer
Copy link
Owner

I think it is a little bit hard to determine when to fold these into a single line. I did some initial test but the result doesn't look good

image
image

@ntjess
Copy link
Author

ntjess commented May 26, 2024

Fair point; perhaps only when an "else" clause is also specified?

@Enter-tainer
Copy link
Owner

Enter-tainer commented May 26, 2024

Fair point; perhaps only when an "else" clause is also specified?

I propose a way to determine whether to inline if or not:

  1. Never inline if statement. That is, ifs that directly appears in code blocks
  2. For other ifs, if it has an else clause and all its subclause only has a single item(and the item is not commet!), and if all these thing can fit in the column width, we put it in a single line.

@ntjess
Copy link
Author

ntjess commented May 26, 2024

That makes sense to me!

For other ifs

Are there additional ifs besides assignment and control statement?

@Enter-tainer
Copy link
Owner

I mean we may only inline ifs that appears in expression, function args, rhs of assignment ... But we shouldn't inline ifs that is directly in a code block.

{
  // DO NOT
  if ... {
  ...
  } else {
  ..
  }
  let a = if ... { } else { } // DO
  let a = f(if ... { } else { }, ...) // DO
  let a = (1, 2, if ... { } else { }) // DO
}

@Enter-tainer Enter-tainer added the good first issue Good for newcomers label May 26, 2024
@Enter-tainer
Copy link
Owner

I just realize that the body of if can be content blocks as well, like

#if 1 > 2 [ 111 ] else [ 222 ]

So maybe we should have a special code path for "simple if"s. If we have a if,

  1. not a statement(direct parent is not code block)
  2. has 2 sub clause
  3. both of sub clause is code block
  4. each sub clause has only one item inside, and the item is not line comment

We try to inline it.

@ntjess
Copy link
Author

ntjess commented May 28, 2024

Content blocks should be acceptable too, right? IMO As long as they are short enough, the cognitive complexity is still lower when inlined.

All the other conditions make sense to me.

@Myriad-Dreamin
Copy link
Collaborator

Myriad-Dreamin commented May 30, 2024

  1. Never inline if statement. That is, ifs that directly appears in code blocks
  2. For other ifs, if it has an else clause and all its subclause only has a single item(and the item is not commet!), and if all these thing can fit in the column width, we put it in a single line.

From my point of view, the first point is good but the second point is not quite good. I don't think it good and robust to check so many conditions: if it has an else clause and all its subclause only has a single item(and the item is not commet!). Why don't we change the perspective for this issue to simplify the rule? inline if expression only if

  1. any expression is on the value position of a named item, such as:
#table( // named item can be as a function argument
  fill: (x, y) => if y == 0 { white.darken(15%) } else { none },
)
#let paint = ( // named item can be as an item of some dictionary literal
  stroke: if y == 0 { white.darken(15%) } else { none },
)
  1. the entire expression's width can fit in the column width.

I think it is a generally great enough rule to make beautiful named items, and we can find other good rules when someone make another issue.

@ntjess
Copy link
Author

ntjess commented May 30, 2024

I think the reduced complexity makes sense, but I would advocate for let declarations to also be considered. To me, readability decreases with spacing for the following example:

#let x = if cond {
  5
} else {
  6
}

However, let introduces all the edge cases from above. So I can understand avoiding it in favor or straightforward rules.

@Enter-tainer
Copy link
Owner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants