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

Add snippet 'foreach-indexed' #2070

Open

Conversation

@travis-c-lagrone
Copy link
Contributor

@travis-c-lagrone travis-c-lagrone commented Jul 6, 2019

PR Summary

Adds a snippet named foreach-indexed.

The foreach-indexed snippet enumerates a collection using a foreach loop, but also tracks the index of the current item in the process. Regarding motivation, the use cases of the foreach-indexed construct (which enumerates element-to-index) are somewhat the opposite of the for construct (which enumerates index-to-element): synthetically indexing an unordered collection (e.g. a HashSet), relatively indexing a live enumerator (vs. an absolutely-indexed enumerable), etc. Regarding justification, the logic sequencing required by the foreach-indexed construct to defend against naive uses of the continue keyword in the middle of the loop body (i.e. continuing without also incrementing the index) is both unconventional as well as more verbose than the conventional pattern (i.e. of incrementing the index at the end of the loop body).

PR Checklist

  • PR has a meaningful title
  • Summarized changes
  • NA PR has tests
  • This PR is ready to merge and is not work in progress
@travis-c-lagrone travis-c-lagrone changed the title Add snippet named 'foreach-indexed' Add snippet 'foreach-indexed' Jul 6, 2019
@rjmholt
Copy link
Member

@rjmholt rjmholt commented Jul 9, 2019

Thanks for your contribution @travis-c-lagrone.

Although this is generally applicable, I think this one is more advanced and specific than other snippets we enable by default and would be better in the community snippets doc.

@rjmholt rjmholt removed the Triage label Jul 9, 2019
@travis-c-lagrone
Copy link
Contributor Author

@travis-c-lagrone travis-c-lagrone commented Jul 10, 2019

@rjmholt Would you plesae clarify what the criteria (even if open-ended) is for whether a snippet is appropriate to be included in the extension? I ask because while I do not understand how this proposed snippet is either more "advanced" or "specific" than many of the other snippets in the extension (e.g. "IfShouldProcess", "CalculatedProperty", "IArgumentCompleter Class", etc.), although I could see how it might be rejected on the basis of less frequent use.

@rjmholt
Copy link
Member

@rjmholt rjmholt commented Jul 10, 2019

It's certainly case-by-case, but here the feeling was that an indexed foreach loop is an uncommon use-case given a standard for-loop and the usage of the private $_index as well as the $index variable made for a complex solution despite being nifty.

Because the JSON snippets represent part of the completions the extension offers, we really want to make sure that those snippets represent PowerShellisms most users would use; so essentially standard structures (like a loop or an advanced function or a switch) that you would find yourself using on a weekly basis and that would see in examples and tutorials in the wild but for which there's no completion.

@travis-c-lagrone
Copy link
Contributor Author

@travis-c-lagrone travis-c-lagrone commented Jul 11, 2019

Ok, I can grok the idea of "PowerShellisms". I'll modify this PR to add the foreach-indexed snippet to the Community Snippets instead.

Also, before I make a PR for it, how would you see a snippet like the following applying to the idea of regularly used PowerShellisms for which there's no completion (much less dedicated command)?

"Test-Any loop": {
    "prefix": "any",
    "body": [
        "\\$any = \\$false",
        "foreach (\\$${1:object} in \\$${2:objects}) {",
        "\tif (${3:\\$$1}) {",
        "\t\t\\$any = \\$true",
        "\t\tbreak",
        "\t}",
        "}",
    ],
},
"Test-All loop": {
    "prefix": "all",
    "body": [
        "\\$all = \\$true",
        "foreach (\\$${1:object} in \\$${2:objects}) {",
        "\tif (-not ${3:\\$$1}) {",
        "\t\t\\$all = \\$false",
        "\t\tbreak",
        "\t}",
        "}",
    ],
},
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.