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

Wrapping at infix operator in long if statement chain #234

Open
gadenbuie opened this issue Feb 19, 2025 · 9 comments
Open

Wrapping at infix operator in long if statement chain #234

gadenbuie opened this issue Feb 19, 2025 · 9 comments

Comments

@gadenbuie
Copy link
Contributor

Just bumped into this in the wild (i.e. code I didn't write in chromote). I expected no change to the following code,

if (
  domain %in% private$session$get_auto_events() &&
    private$event_callback_counts[[domain]] == 1 &&
    isTRUE(private$event_enable_domains[[domain]])
) {
  private$session$debug_log("Enabling events for ", domain)
  private$session[[domain]]$enable()
}

but air format introduces a line break after the %in%:

if (
  domain %in%
    private$session$get_auto_events() &&
    private$event_callback_counts[[domain]] == 1 &&
    isTRUE(private$event_enable_domains[[domain]])
) {
  private$session$debug_log("Enabling events for ", domain)
  private$session[[domain]]$enable()
}

It's not a new change, this repros back to air 0.1.1.

@gadenbuie
Copy link
Contributor Author

Smaller reprex:

# before
if (x %in% letters && this_needs_to_be_long_enough && this_also_needs_to_be_long_enough) {
  'result'
}

# after
if (
  x %in%
    letters &&
    this_needs_to_be_long_enough &&
    this_also_needs_to_be_long_enough
) {
  'result'
}

@lionel-
Copy link
Collaborator

lionel- commented Feb 19, 2025

hmm I think this is to be expected?

@gadenbuie
Copy link
Contributor Author

This seems inconsistent

# before
if (x %in% letters && y %in% some_other_collection_of_letters && x == who_knows_what) {
  'result'
}

# after
if (
  x %in%
    letters &&
    y %in% some_other_collection_of_letters &&
    x == who_knows_what
) {
  'result'
}

@kpagacz
Copy link

kpagacz commented Mar 1, 2025

Out of the box, tergo formats this the way you want it:

if (
  domain %in% private$session$get_auto_events() &&
    private$event_callback_counts[[domain]] == 1 &&
    isTRUE(private$event_enable_domains[[domain]])
) {
  private$session$debug_log("Enabling events for ", domain)
  private$session[[domain]]$enable()
}

and the smaller reprex:

# before
if (
  x %in% letters &&
    this_needs_to_be_long_enough &&
    this_also_needs_to_be_long_enough
) {
  "result"
}

# after
if (
  x %in% letters &&
    this_needs_to_be_long_enough &&
    this_also_needs_to_be_long_enough
) {
  "result"
}

Check out tergo (it also has an R package) if you haven't already! (I am the author!)

@jayhesselberth
Copy link

+1 for no break after infix operators.

The break makes the lhs and rhs look like operands, esp. mixed with && or ||.

@DavisVaughan
Copy link
Collaborator

@kpagacz tergo formats this similar to air with line_length = 80.

# before
if (x %in% letters && this_needs_to_be_long_enough && this_also_needs_to_be_long_enough) {
  'result'
}

# after (tergo)
if (
  x %in%
    letters &&
    this_needs_to_be_long_enough &&
    this_also_needs_to_be_long_enough
) {
  'result'
}

# after (air)
if (
  x %in%
    letters &&
    this_needs_to_be_long_enough &&
    this_also_needs_to_be_long_enough
) {
  'result'
}
# before
if (x %in% letters && y %in% some_other_collection_of_letters && x == who_knows_what) {
  'result'
}

# after (tergo)
if (
  x %in%
    letters &&
    y %in%
      some_other_collection_of_letters &&
    x ==
      who_knows_what
) {
  'result'
}

# after (air)
if (
  x %in%
    letters &&
    y %in% some_other_collection_of_letters &&
    x == who_knows_what
) {
  'result'
}
# With `line_length = 80`
tergo ~/Desktop/test2.R ~/Desktop/tergo.toml

@DavisVaughan
Copy link
Collaborator

DavisVaughan commented Mar 1, 2025

@jayhesselberth %>% is an infix operator similar to |>. %||% is an infix operator similar to ||. %*% is an infix operator similar to *. I'd argue those should chain in the same way as other non-infix operators (certainly for the pipe). %in% is the one that feels weird here, I think.

@jayhesselberth
Copy link

Yeah, maybe %in% just needs special treatment.

@kylebutts
Copy link

I have other examples of this.

Formatted:

tibble(
  tmdb_production_company_name = res$production_companies[[1]]$name,
  tmdb_production_company_country = res$production_companies[[
    1
  ]]$origin_country
)

new_var_with_very_long_name_to_cause_overflow <- (1.2382 +
  20.3402 +
  12120.3402 * 2) /
  sqrt(20.3920^2 + 5.1292^2)

My preference would be for it to look something like

tibble(
  tmdb_production_company_name = res$production_companies[[1]]$name,
  tmdb_production_company_country = 
    res$production_companies[[1]]$origin_country
)

new_var_with_very_long_name_to_cause_overflow <- 
  (1.2382 + 20.3402 + 12120.3402 * 2) /
  sqrt(20.3920^2 + 5.1292^2)

I think a general rule would be to see if you could fit in a new line, to not fully expand?

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

6 participants