Skip to content
This repository has been archived by the owner on Jul 1, 2024. It is now read-only.

Add binary operators #7

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

JamesWTruher
Copy link

No description provided.

@kiennq kiennq mentioned this pull request Aug 26, 2022
3 tasks
@tgross35
Copy link

tgross35 commented Sep 9, 2023

@rjmholt would you be able to review this PR? I think it should resolve #8

@ferplnat
Copy link

ferplnat commented Oct 3, 2023

+1 it would be nice to be able to build this off of the official repo.

Copy link
Contributor

@rjmholt rjmholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a note about the generality of the expressions under the binop, but this looks like a good improvement in any case

),

binary_operator_statement: $ => seq(
$.variable,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have expected this to also allow an expression on the LHS. I see the tests only look variable binop comparisons, rather than something like (gci | % { $_.filepath }) -join "`n"

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is changing $.variable to $._expression the only thing needed here? I can do that afterward, or I think you might be able to suggest & commit the change directly to this PR as a maintainer.

In any case, would you consider merging this? Even without this change, mostly not broken is a huge improvement over the current totally broken state :)

@tgross35
Copy link

tgross35 commented Oct 10, 2023

@JamesWTruher are you still able to work on this? (No pressure, just wondering if you're still active)

@tgross35
Copy link

tgross35 commented Jan 9, 2024

@rjmholt can you merge this as-is? I can do an update PR afterward to fix what you mentioned.

@rjmholt
Copy link
Contributor

rjmholt commented Jan 9, 2024

AFAIK I don't have merge privileges

@tgross35
Copy link

tgross35 commented Jan 9, 2024

Oh, it looked like you had most of the git history 🙃

Do you know anyone who would?

@tgross35
Copy link

tgross35 commented Jan 9, 2024

Maybe @TylerLeonhardt - are you able to merge?

@TylerLeonhardt
Copy link
Member

Alas, I do not... but @SteveL-MSFT should!

@tgross35
Copy link

tgross35 commented Jan 9, 2024

No wonder there haven't been any updates in 5 years, nobody can push :)

@tgross35
Copy link

Gentle ping @SteveL-MSFT or anyone else who has permissions - could this get merged?

It would be probably be good if anyone with settings access could add another maintainer (@rjmholt?) to help get this repo back up to date. And/or somebody from the TS side, @amaanq maintains a ton of grammars and might be willing to or might know who to ask.

@JunHaoShih
Copy link

So this is basically a dead repo?

@cjhillbrand
Copy link

@JunHaoShih @tgross35, I just shot him an email directly, asking him to assign me write permission to the repo. Hopefully at that point we can begin to unblock a few of these PRs, etc

@amaanq
Copy link

amaanq commented Feb 29, 2024

Might want to check this out, seems more maintained https://github.com/airbus-cert/tree-sitter-powershell

@tgross35
Copy link

tgross35 commented Mar 7, 2024

Any luck getting in touch @cjhillbrand?

Might want to check this out, seems more maintained https://github.com/airbus-cert/tree-sitter-powershell

Yeah, I know that one is definitely more active. It just makes sense that the PowerShell syntax in the PowerShell org would be the de facto one, maybe effort can be pooled if a new maintainer can be added here.

Or just archive this and point to that repo... but then you would still need a maintainer to update the readme here 🙂

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants