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 initial set support #36

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

Add initial set support #36

wants to merge 4 commits into from

Conversation

Neved4
Copy link
Contributor

@Neved4 Neved4 commented Nov 16, 2024

Fixes #34 #35

Would love some guidance to create the tests.

Note: Currently doesn't support set -- x y "$@" to append. This is equivalent to fish's set -a argv x y

@Neved4 Neved4 changed the title Add initial support for set constructs Add initial set support Nov 16, 2024
@bouk
Copy link
Owner

bouk commented Nov 18, 2024

Nice, just some tests :)

@Neved4
Copy link
Contributor Author

Neved4 commented Nov 18, 2024

Done!

@@ -333,6 +333,8 @@ echo ${cool+a}
echo ${cool:+a}
echo ${cool-a}
echo ${cool:-a}
set -Cefu
Copy link
Owner

Choose a reason for hiding this comment

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

Now we're just silently ignoring this, maybe that's not good? Can we support some of these options? Or is there no fish equivalent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think there's any fish equivalent for these, see the picture in: #34 (comment).

And the later description:

Of these, -C doesn't exist in fish, and -e, -f, -u all have different meanings.

As-is they'll break fish scripts, so my approach is to filter them out.

For more detail, someone writing set -e in UNIX shells is expecting to terminate the script early on errors, with some caveats. Someone writing set -e on fish is expecting to erase a shell variable, akin to what would be unset.

@Neved4
Copy link
Contributor Author

Neved4 commented Nov 19, 2024

I'm yet to support set - x as opposed to set -- x.

set -- x "$@" into set -a argv x looks more challenging, but I'll still give it a try.

Makes sense to add those two translations to this PR.

Checklist:

  • set - x -> set argv x
  • set -- x -> set argv x
  • set -- x "$@" -> set -a argv x
  • set -Cefu -> , empty string

@Neved4
Copy link
Contributor Author

Neved4 commented Nov 19, 2024

Supported set -x, but stuck on this last one.

Since $@ is globally transformed into argv in the parser, it'll still return set argv x $argv.

Tried adding more logic to parseSetExpr(), but unsure how to skip the $@ -> argv translation for specific lines.

If we skip it for lines akin to set -- x "$@", we can translate it to the correct fish output set -a argv x.

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.

Filter out shell set -$flag calls
2 participants