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

Support optional else-branch for if-then-elif-end #2598

Merged
merged 1 commit into from
Jul 3, 2023

Conversation

wader
Copy link
Member

@wader wader commented Jun 1, 2023

Fixes #2481

@wader
Copy link
Member Author

wader commented Jun 1, 2023

Not sure if seen as bug fix or feature, but can maybe wait until after current master has been released.

src/parser.y Outdated
@@ -619,6 +619,9 @@ ElseBody:
"elif" Exp "then" Exp ElseBody {
$$ = gen_cond($2, $4, $5);
} |
"elif" Exp "then" Exp "end" {
$$ = gen_cond($2, $4, gen_noop());
} |
Copy link
Member Author

Choose a reason for hiding this comment

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

This can probably be done in some cleverer way but this way feels straight forward

Copy link
Contributor

Choose a reason for hiding this comment

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

Having optional then in different places looks confusing. You can move both in ElseBody;

Exp:
...
"if" Exp "then" Exp ElseBody {
  $$ = gen_cond($2, $4, $5);
} |
"if" Exp "then" error {
  FAIL(@$, "Possibly unterminated 'if' statement");
  $$ = $2;
} |
...

ElseBody:
"elif" Exp "then" Exp ElseBody {
  $$ = gen_cond($2, $4, $5);
} |
"else" Exp "end" {
  $$ = $2;
} |
"end" {
  $$ = gen_noop();
}

@wader
Copy link
Member Author

wader commented Jun 1, 2023

@owenthereal CI for a PR seems to work

Copy link
Member

@owenthereal owenthereal left a comment

Choose a reason for hiding this comment

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

:shipit:

@wader
Copy link
Member Author

wader commented Jun 2, 2023

I think we should maybe wait with any changes that are not absolutely needed for next release or what do ppl think? @itchyny @liquidaty

@liquidaty
Copy link

Personally I think it would be nice to see changes incorporated in stages and/or separate releases:

  1. Pure QC/CI/CD. In theory, the artifacts should not change, and only the distribution mechanism (i.e. where to pick up binaries) changes
  2. Bug fixes
  3. Simple enhancements that can be made to only apply on an opt-in basis such as comment stripping-- basic PR here (yes selfish, I know)
  4. New functions (e.g. utf8 upper/lower)
  5. Other changes such as those that impact syntax (e.g. if-elif-option-else)

@itchyny itchyny added this to the 1.7 release milestone Jun 25, 2023
@wader
Copy link
Member Author

wader commented Jul 1, 2023

Rebased on master. macOS build fail seems to be some homebrew temporary error

src/parser.y Outdated
@@ -619,6 +619,9 @@ ElseBody:
"elif" Exp "then" Exp ElseBody {
$$ = gen_cond($2, $4, $5);
} |
"elif" Exp "then" Exp "end" {
$$ = gen_cond($2, $4, gen_noop());
} |
Copy link
Contributor

Choose a reason for hiding this comment

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

Having optional then in different places looks confusing. You can move both in ElseBody;

Exp:
...
"if" Exp "then" Exp ElseBody {
  $$ = gen_cond($2, $4, $5);
} |
"if" Exp "then" error {
  FAIL(@$, "Possibly unterminated 'if' statement");
  $$ = $2;
} |
...

ElseBody:
"elif" Exp "then" Exp ElseBody {
  $$ = gen_cond($2, $4, $5);
} |
"else" Exp "end" {
  $$ = $2;
} |
"end" {
  $$ = gen_noop();
}

@wader
Copy link
Member Author

wader commented Jul 3, 2023

@itchyny Thanks, yes that is better

@wader wader requested a review from itchyny July 3, 2023 14:59
Copy link
Contributor

@itchyny itchyny left a comment

Choose a reason for hiding this comment

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

LGTM!

@itchyny itchyny merged commit 12ce4e3 into jqlang:master Jul 3, 2023
14 checks passed
@wader wader deleted the if-elif-optional-else branch July 3, 2023 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Elif and optional else not working
4 participants