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

Fix parsing of def using flags #74

Merged
merged 2 commits into from
Jan 25, 2024
Merged

Conversation

kit494way
Copy link
Contributor

Fix #49

@fdncred
Copy link
Collaborator

fdncred commented Jan 24, 2024

I see these tested

def --env foo [] {}
def --env --wrapped bar [...args] {}

but does it cover these?

def --wrapped --env foo [] {}
def --wrapped bar [...args] {}

@stevenxxiu
Copy link

I also wonder if def-env should be changed to def --env?

@fdncred
Copy link
Collaborator

fdncred commented Jan 24, 2024

@stevenxxiu I don't follow. Where is def-env used in this PR?

@stevenxxiu
Copy link

stevenxxiu commented Jan 24, 2024

@fdncred There are some in the tests and some in grammar.js, but this is now removed.

My changes based on @kit494way, at 750dab7 if you're interested.

@fdncred
Copy link
Collaborator

fdncred commented Jan 24, 2024

if we still have def-env anywhere, it needs to be removed, but not necessarily in this PR.

@stevenxxiu
Copy link

stevenxxiu commented Jan 24, 2024

That could make sense, but would it be better to directly change the def-env tests to def --env, and remove def-env at the same time? After all, I thought the purpose of def flags is to replace def-env.

@kit494way
Copy link
Contributor Author

@fdncred @stevenxxiu Thanks for your reviews. I will remove def-env and fix the tests in this PR.

@kit494way
Copy link
Contributor Author

I removed def-env and fixed the tests.

@fdncred
Copy link
Collaborator

fdncred commented Jan 25, 2024

Thanks. looks good!

@fdncred fdncred merged commit 2d0dd58 into nushell:main Jan 25, 2024
3 checks passed
@kit494way kit494way mentioned this pull request Jan 29, 2024
@kit494way kit494way deleted the fix-parsing-def branch February 3, 2024 06:10
mrdgo pushed a commit to mrdgo/tree-sitter-nu that referenced this pull request Sep 23, 2024
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.

Parsing of def with flags fails
3 participants