Cover flatshapes

[AucaCoyan]

All right! That took some time 🕙. Sorry for the long wait, I had life coming in the middle.
Anywho, I finished a bit more of FlatShapes

• FlatShape::StringInterpolation
• FlatShape::Block
• FlatShape::Closure
• FlatShape::InternalCall(declid)
• FlatShape::External
• FlatShape::ExternalArg
• FlatShape::Signature
• FlatShape::Keyword
• FlatShape::VarDecl(varid)
• FlatShape::Variable

There are a bunch more to solve, but most importantly, the code is screaming for a refactor.
The issue is that I used more and more booleans to do quick checks "is this coming from a def keyword?" "are we in a string interpolation line?" and so on, that it gets very spaguetti if we dont use smarter solutions like peek for example

As a conclusion, I am ready for this PR, but I know now that the direction is not to include all the remaining FlatShapes, but reorganize the code a little better 😇

[AucaCoyan]

Currently, the nufmt has to strip all the whitespace and breaklines as possible, so then we can work into indentations and proper line length. I started with good code so I can see what it nufmt does with it.
The most important thing in all of the examples is that nufmt does not break code! 🪂

I'm advancing, I can give a few examples:
1️⃣ from this file

export def-env goto [
    query?: string  # a search query to narrow down the list of choices
] {
    let choice = (
        pick repo 
        $"Please (
        ansi yellow_italic)choose a repo(ansi reset) to (ansi green_underline)jump to:(ansi reset)"
        $query
    )
    if ($choice | is-empty) { return }
    cd (get root dir | path join $choice)
}

nufmt outputs

export def-env goto [
    query?: string  # a search query to narrow down the list of choices
] { let choice = ( pick repo $"Please (ansi yellow_italic )choose a repo(ansi reset ) to (ansi green_underline )jump to:(ansi reset )"$queryy) 
if ( $choice | is-empty ) { return } 
cd ( get root dir | path join $choice ) } 

sample 2️⃣ :

# fuzzy-delete any repository managed by `gm`
export def remove [
    query?: string      # a search query to narrow down the list of choices
    --force (-f): bool  # do not ask for comfirmation when deleting a repository
] {
    let choice = (pick repo
        $"Please (ansi yellow_italic)choose a repo(ansi reset) to (ansi red_underline)completely remove:(ansi reset)"
        $query
    )
    if ($choice | is-empty) {
        return
    }

    let repo = (get root dir | path join $choice)
    if $force {
        rm --trash --verbose --recursive $repo
    } else {
        rm --trash --verbose --recursive $repo --interactive
    }
}

to this

# fuzzy-delete any repository managed by `gm`
export def remove [
    query?: string      # a search query to narrow down the list of choices
    --force (-f): bool  # do not ask for comfirmation when deleting a repository
] { 
let choice = ( pick repo $"Please (ansi yellow_italic )choose a repo(ansi reset ) to (ansi red_underline )completely remove:(ansi reset )"$query ) 
if ( $choice | is-empty ) { return } 
let repo = ( get root dir | path join $choice ) 
if $force { rm --trash --verbose --recursive $repo } else { rm --trash --verbose --recursive $repo --interactive } } 

and the final 3️⃣ :

# get windows service status
def get-win-svc [] {
  sc queryex type=service state=all | 
    collect {|x| $x | 
    parse -r '(?m)SERVICE_NAME:\s*(?<svc>\w*)\s*DISPLAY_NAME:\s*(?<dsp>.*)\s*TYPE\s*:\s*(?<type>[\da-f]*)\s*(?<typename>\w*)?\s*\s*STATE\s*:\s*(?<state>\d)\s*(?<status>\w*)\s*(?<state_opts>\(.*\))?\s*?WIN32_EXIT_CODE\s*:\s*(?<exit>\d*).*\s*SERVICE_EXIT_CODE\s*:\s*(?<svc_exit>\d)\s*.*\s*CHECKPOINT\s*:\s*(?<chkpt>.*)\s*WAIT_HINT\s*:\s(?<wait>.*)\s*PID\s*:\s*(?<pid>\d*)\s*FLAGS\s*:\s(?<flags>.*)?' | 
    upsert status {|s| 
      if $s.status == RUNNING { 
        $"(ansi green)●(ansi reset)" 
      } else { 
        $"(ansi red)●(ansi reset)" 
      }
    } | into int state exit svc_exit chkpt wait pid
    }
}

and the result

# get windows service status
def get-win-svc [] { sc queryex type=service state=all | collect {|x | $x | parse -r '(?m)SERVICE_NAME:\s*(?<svc>\w*)\s*DISPLAY_NAME:\s*(?<dsp>.*)\s*TYPE\s*:\s*(?<type>[\da-f]*)\s*(?<typename>\w*)?\s*\s*STATE\s*:\s*(?<state>\d)\s*(?<status>\w*)\s*(?<state_opts>\(.*\))?\s*?WIN32_EXIT_CODE\s*:\s*(?<exit>\d*).*\s*SERVICE_EXIT_CODE\s*:\s*(?<svc_exit>\d)\s*.*\s*CHECKPOINT\s*:\s*(?<chkpt>.*)\s*WAIT_HINT\s*:\s(?<wait>.*)\s*PID\s*:\s*(?<pid>\d*)\s*FLAGS\s*:\s(?<flags>.*)?' | upsert status {|s| 
      if $s.status == RUNNING { 
        $"(ansi green)●(ansi reset)" 
      } else { 
        $"(ansi red)●(ansi reset)" 
      }
    } | into int state exit svc_exit chkpt wait pid } } 

with the final example I realized some problems, the FlatShape::Closure is not working as intended. The nufmt inputs and outputs almost the same, which not the expected result.
Also, the software still needs some tuning to add spaces in the correct order, but those are details in comparison to other breaking issues. Step by step we are moving on!

[amtoine]

@AucaCoyan

let choice = ( pick repo $"Please (ansi yellow_italic )choose a repo(ansi reset ) to (ansi red_underline )completely remove:(ansi reset )"$query )

this does not look right to me 🤔
the $query being right next to the end of the previous string 😕

also in the first example, i assume the double y in $queryy is just a bad copy-paste?

Step by step we are moving on!

good to hear that 😌

[AucaCoyan]

the $query being right next to the end of the previous string 😕
also in the first example, i assume the double y in $queryy is just a bad copy-paste?

Yes, that doesn't look great. We need to dial in some details. queryy was a typo 😋

[amtoine]

Yes, that doesn't look great. We need to dial in some details.

at least if it's still correct 🤞

queryy was a typo yum

glad to hear that 😉

[amtoine]

a few minor remarks and questions but looks good 👌

the tests already did pass before and still pass now.
i'm wondering, as you are covering more of the language, if new tests reflecting these new flatshapes should be added?

[amtoine]

these values come from Nushell itself, right?

[AucaCoyan]

Yes, they come from nushell, but I didn't found them on the repo (I am convinced that they are there, but I didn't find them).
I made that list in base of trial and error, writing down the keywords that gave me problems and reading the log to see what happens

[amtoine]

mm yeah, true, i don't see them in the code base of Nushell 🤔

[amtoine]

we can use these for now, and if we see a more robust way to specify them later... 😉

[sholderbach]

I don't think you can depend on those IDs being constant. effectively they are generated by when a particular builtin is pushed into the working_set. So if someone would rearrange the create_default_context() (or whatever it is called now for nu-cmd-lang) this would break.

With the vision for new-nu-parser that those are clear keywords and maybe even don't have an underlying command things could become easier.

Maybe as a bridging solution we could first introduce a FlatShape::KeywordCall or similar on the Nu-side that has a guaranteed mapping in the flattening.

[AucaCoyan]

That sounds great!
I actually should write the formatter based on the new parser, shouldn't I? The new parser is on course and will eventually come. Of course because it's not finished there may be some tokens or kewords I could not format yet, but a least with very simple cases I can build a formatter and adapt in the way, what do you think?

[AucaCoyan]

if new tests reflecting these new flatshapes [or new tests] should be added?

Yes! the existing tests reflect every of these combinations. Before we didn't "looked into" closures and definitions, but now that we do, a lot of asserts went bad on the first run. A couple of lines after, I made the changes to see CI in green 😄

[sholderbach]

I think we shouldn't stall experimental progress here so the DeclId should not strictly block this but we need to make a change on the nu side soon so this is not breaking on changes in nushell.

[AucaCoyan]

Hello! Hope you are doing well 😄
This PR got a little stalled imo. What do you think about merging this and start with the new design of the upcoming parser?
I don't want to hurry at all! but I think we have arrived to a stalemate among DeclId, FlatShape::KeywordCall and the new parser, what do you think?

[amtoine]

agree @AucaCoyan, let's move forward with this 😌

[AucaCoyan]

Thank you so much!
This was an enormous rock for me in this project. This PR waited months. I finally did it 😭😀

[fdncred]

wow, a 6 month long pr finally landed. good job!