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

Revert: Restrict closure expression to be something like {|| ...} #8636

Closed
wants to merge 1 commit into from

Conversation

WindSoilder
Copy link
Collaborator

Description

This reverts pr: #8290
reopen: #7921
reopen: #8273

According to message from @jntrnr : because it'll break everyone's config file, and we may come up with a different syntax.

User-Facing Changes

(List of all changes that impact the user experience here. This helps us keep track of breaking changes.)

Tests + Formatting

Don't forget to add tests that cover your changes.

Make sure you've run and fixed any issues with these commands:

  • cargo fmt --all -- --check to check standard code formatting (cargo fmt --all applies these changes)
  • cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect to check that you're using the standard code style
  • cargo test --workspace to check that all tests pass

Note
from nushell you can also use the toolkit as follows

use toolkit.nu  # or use an `env_change` hook to activate it automatically
toolkit check pr

After Submitting

If your PR had any user-facing changes, update the documentation after the PR is merged, if necessary. This will help us keep the docs up to date.

@sophiajt
Copy link
Contributor

We're talking this one through now to see if we want to leave it or revert it. If we haven't landed it by this week's team meeting, I'm guessing we'll talk more about it then.

@fdncred
Copy link
Collaborator

fdncred commented Mar 27, 2023

It seems like there is some issue around this area too that was breaking if {} else if {} where else if was being detected wrong. We can't revert this and have else if issues again, imo.

@fdncred
Copy link
Collaborator

fdncred commented Mar 27, 2023

what if we allowed a closure without || if it used $in, otherwise, it's required to have a named closure variable? For instance, these items in my them could work without || because they use $in, like this.

  bool: { if $in { 'light_cyan' } else { 'light_red' } }
  date: { (date now) - $in |
    if $in < 1hr {
      'red3b' 
    } else if $in < 6hr {
      'orange3' 
    } else if $in < 1day {
      'yellow3b' 
    } else if $in < 3day {
      'chartreuse2b' 
    } else if $in < 1wk {
      'green3b' 
    } else if $in < 6wk {
      'darkturquoise' 
    } else if $in < 52wk {
      'deepskyblue3b' 
    } else { 'dark_gray' }
  }

while this one would still work as normal since it uses |e|

  filesize: {|e| if $e == 0b { 'white' } else if $e < 1mb { 'blue' } else { 'cyan' } }

Would that work or would it be more more confusing, or just impossible to code?

@sophiajt
Copy link
Contributor

@fdncred - unfortunately, that'd mean collecting the stream currently.

@sophiajt
Copy link
Contributor

We talked about this in today's design meeting and decided against reverting the syntax. We'll go ahead with shipping the change to require {|| } for closures.

@WindSoilder
Copy link
Collaborator Author

Yeah, so it can be closed

@WindSoilder WindSoilder deleted the no_closure_for_now branch August 2, 2023 22:06
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.

None yet

3 participants