Allow 'else if' construct #378

czerwinskilukasz1 · 2020-07-09T13:40:30Z

Currently we don't have switch nor we permit else if, because else expects a block with curly braces.
How about we permit else if, such as:

    if (len == 0) {
      return 'no one likes it';
    } else if (len == 1) {
      return concat(names[0], ' likes it');
    } else if (len == 2) {
      return concat(names[0], ' and ', names[1], ' like it');
    } else if (len == 3) {
      return concat(names[0], ', ', names[1], ' and ', names[2], ' like it');
    } else {
      return concat(names[0], ', ', names[1], ' and ', len, ' others like it');
    }

The text was updated successfully, but these errors were encountered:

czerwinskilukasz1 · 2020-07-09T13:40:53Z

@mhagmajer

mhagmajer · 2020-07-09T13:48:05Z

Yes, else if would definitely be nice. Once we have type checking we could look into pattern matching like https://ocaml.org/learn/tutorials/data_types_and_matching.html#Pattern-matching-on-datatypes

czerwinskilukasz1 · 2020-07-09T15:36:14Z

Yes, else if would definitely be nice.

Great.

czerwinskilukasz1 · 2020-07-09T15:36:28Z

Task confirmed.

czerwinskilukasz1 · 2020-07-09T15:36:34Z

Once we have type checking we could look into pattern matching like https://ocaml.org/learn/tutorials/data_types_and_matching.html#Pattern-matching-on-datatypes

Looks to me similar to pattern matching in Scala: https://docs.scala-lang.org/tour/pattern-matching.html

markkulube · 2020-07-10T19:52:05Z

@czerwinskilukasz1 i would like to look into this!

czerwinskilukasz1 · 2020-07-13T12:13:22Z

Hello @markkulube , sounds great!

czerwinskilukasz1 · 2020-07-13T12:14:23Z

If you had any questions, feel free to ask them here (tag me please) or on our Discord channel.

markkulube · 2020-07-15T10:08:05Z

@czerwinskilukasz1 thank you. A couple questions to help me better navigate through the project.

Still at the dawn of contributing to big projects so please bear with me.

Is it to add an ElseIf class and an | ElseIf statement to the Statement class in:
src/askscript/parser/askscript.grammar.pegjs.classes.ts
Then create a functional component ElseIf.tsx with path:
src/askjsx/lib/ElseIf.tsx
Here we are defining ElseIf statement similar to that of If
src/askscript/parser/askscript.grammar.pegjs

czerwinskilukasz1 · 2020-07-15T14:23:37Z

@markkulube , it's all good and your questions are 100% appropriate!

There is a couple of ways you could handle it. Given the solution you proposed, you analyzed the codebase and understood a good chunk of its structure, congrats 🙂
Your solution should work well, but it requires changing 3 files in 2 modules. There is actually a bit simpler way, which is handling else if as a syntax sugar. What does it mean? It means translating the new syntax into existing askjsx structures.

So I propose to:

Define an ElseIf statement, as you mentioned
src/askscript/parser/askscript.grammar.pegjs
Add ElseIf class with print() method which would output the same as if else { if(...) { } } was used. Review other print() methods to find inspirations on how to perform such conversions.

What do you think?

czerwinskilukasz1 · 2020-07-30T01:23:03Z

Hi Mark, did you have a while to think over the change?
@markkulube

markkulube · 2020-07-30T15:24:11Z

@czerwinskilukasz1 my deepest apologies. work has been pretty busy for the past couple weeks. the good thing is my current project is MERN stack as well and it's a long weekend for me here. my plan is to work on this issue to completion as of tomorrow and hopefully close this by next Friday at the latest.

i will be in touch here with a couple question very very soon.

so sorry for the delay!

czerwinskilukasz1 · 2020-07-30T16:14:40Z

@markkulube , hey, thanks for your reply.
Keeping fingers crossed for your MERN project and for the PR for AskQL ;)

markkulube · 2020-08-02T04:56:01Z

hey @czerwinskilukasz1, thanks.

PLEASE DISREGARD

this has been solved as the project now builds locally with my changes. I simply had a mismatch between the ElseIf in variable definitions and elseif under control flow.

as per your recommendations i made a changes in two files but on running npm run build i get an exception from the first few tests it seems:

What would this error indicate: GrammarError: Rule "ElseIf" is not defined.???

PASS build src/hello.ask
FAIL build src/askscript/parser/askscript.grammar.pegjs
● saves /.../XFAANG/dist/askscript/parser/askscript.grammar.js

**GrammarError: Rule "ElseIf" is not defined.**

  at rule_ref (node_modules/pegjs/lib/compiler/passes/report-undefined-rules.js:12:15)
  at visit (node_modules/pegjs/lib/compiler/visitor.js:10:35)
  at node_modules/pegjs/lib/compiler/visitor.js:26:17
  at Object.each (node_modules/pegjs/lib/utils/arrays.js:63:7)
  at node_modules/pegjs/lib/compiler/visitor.js:25:16
  at visit (node_modules/pegjs/lib/compiler/visitor.js:10:35)
  at visitExpression (node_modules/pegjs/lib/compiler/visitor.js:18:13)
  at visit (node_modules/pegjs/lib/compiler/visitor.js:10:35)
  at visitExpression (node_modules/pegjs/lib/compiler/visitor.js:18:13)
  at visit (node_modules/pegjs/lib/compiler/visitor.js:10:35)

PASS build src/hello.ask.formatted.ask
PASS build src/askExpressMiddleware/askExpressMiddleware.spec.ts

src/askscript/parser/askscript.grammar.pegjs

In statement_nows

(line 84) added
-- / ElseIf

In // === control flow ===

(line 213) added
-- elseif = 'else if' '(' ws* v:value ws* ')' ws* cB:codeBlockWithBraces eB:elseif? { return new ask.ElseIf(v, cB, eB) }

src/askscript/parser/askscript.grammar.pegjs.classes.ts
3. Defined class ElseIf

markkulube · 2020-08-02T05:22:00Z

@czerwinskilukasz1 there were 8+12=20 more tests after building and testing with my changes....i have not written any tests myself - is this to be expected?

Keeping in mind my local master branch is up to date with your master branch:

Test Results Before My Changes:
Test Suites: 264 passed, 264 total
Tests: 27 todo, 713 passed, 740 total
Snapshots: 0 total
Time: 43.124 s
Ran all test suites.

Test Results After My Changes:
Test Suites: 272 passed, 272 total
Tests: 27 todo, 725 passed, 752 total
Snapshots: 0 total
Time: 20.465 s
Ran all test suites.

czerwinskilukasz1 · 2020-08-03T23:40:16Z

@czerwinskilukasz1 there were 8+12=20 more tests after building and testing with my changes....i have not written any tests myself - is this to be expected?

Keeping in mind my local master branch is up to date with your master branch:

Test Results Before My Changes:
Test Suites: 264 passed, 264 total
Tests: 27 todo, 713 passed, 740 total
Snapshots: 0 total
Time: 43.124 s
Ran all test suites.

Test Results After My Changes:
Test Suites: 272 passed, 272 total
Tests: 27 todo, 725 passed, 752 total
Snapshots: 0 total
Time: 20.465 s
Ran all test suites.

This is interesting. Possibly a bug? @mhagmajer wrote the scripts which run tests. Could you take a look?

czerwinskilukasz1 · 2020-08-03T23:41:21Z

And congrats for solving the build issue :)

markkulube · 2020-08-04T00:25:50Z

@czerwinskilukasz1 thanks, i am a little stuck on constructing an "if else-if else" block in console, I can only get the if statement going:

🦄 ask(if(5), return('yes'))
string ask(call(get('ask'),call(get('if'),5),call(get('return'),'yes')))
'yes'

What would the askQL expression involving an else-if clause look like?

after i confirm that my changes do work in console then i can move onto writing formal tests then PR.

czerwinskilukasz1 · 2020-08-04T17:41:30Z

Ah, OK, so, the first thing is that ask(if(5), return('yes')) is AskCode (let's think of it as a machine language in AskQL project), while else if construct should be introduce in AskScript, which is the user-facing language.

I pasted in #378 (comment) an example showing how else if construct should look, it is written in AskScript:

    if (len == 0) {
      return 'no one likes it';
    } else if (len == 1) {
      return concat(names[0], ' likes it');
    } else if (len == 2) {
      return concat(names[0], ' and ', names[1], ' like it');
    } else if (len == 3) {
      return concat(names[0], ', ', names[1], ' and ', names[2], ' like it');
    } else {
      return concat(names[0], ', ', names[1], ' and ', len, ' others like it');
    }

markkulube · 2020-08-05T20:32:00Z

@czerwinskilukasz1 my apologies, my question was unclear. i need help in understanding what exact steps i can take to check if my logic is correct.

how can simulate the use case as though I was say in a python shell? something like

suppose names[0] = 'charlie'
>>> len = 2
>>>if (len == 0) {
return 'no one likes it';
} else if (len == 1) {
return concat(names[0], ' likes it');
} else if (len == 2) {
return concat(names[0], ' and ', names[1], ' like it');
} else if (len == 3) {
return concat(names[0], ', ', names[1], ' and ', names[2], ' like it');
} else {
return concat(names[0], ', ', names[1], ' and ', len, ' others like it');
"charlie likes it"
>>>

czerwinskilukasz1 · 2020-08-10T15:21:40Z

@markkulube
Aaaaah, yeah, I missed that.
So, there is a magic command: .editor, which switches CLI to multiline 🙂

🦄 .editor
// Entering editor mode (^D to finish, ^C to cancel)
if (len == 0) {
return 'no one likes it';
} else if (len == 1) {
return concat(names[0], ' likes it');
} else if (len == 2) {
return concat(names[0], ' and ', names[1], ' like it');
} else if (len == 3) {
return concat(names[0], ', ', names[1], ' and ', names[2], ' like it');
} else {
return concat(names[0], ', ', names[1], ' and ', len, ' others like it');
}

Does it answer your question?

markkulube · 2020-08-11T06:43:28Z

@czerwinskilukasz1 yes it worked, thank you. The following tests were executed in console and the output in bold is what we would expect when comparing integer types. It doesn't look like I broke your build/actual tests. Let me know what you think, and my what could be my possible next steps:

###test 1A
let len = 3
if(len == 2) {
return '1A FAILED';
} else {
return '1A PASSED';
}
string ask(let('len',3),if(call(get('=='),get('len'),2),block(return('1A FAILED')),block(list(return('1A PASSED')))))
'1A PASSED'
🦄

###test 1B
let len = 2
if(len == 2) {
return '1B PASSED';
} else {
return '1B FAILED';
}
string ask(let('len',2),if(call(get('=='),get('len'),2),block(return('1B PASSED')),block(list(return('1B FAILED')))))
'1B PASSED'
🦄

###test 2A
let len = 10
if(len == 2) {
return '2A FAILED';
} else if(len == 10) {
return '2A PASSED';
} else {
return '2A FAILED';
}
string ask(let('len',10),if(call(get('=='),get('len'),2),block(return('2A FAILED')),block(if(call(get('=='),get('len'),10),block(return('2A PASSED')),block(list(return('2A FAILED')))))))
'2A PASSED'
🦄

###test 2B
let len = 200
if(len == 200) {
return '2B PASSED';
} else if(len == 10) {
return '2B FAILED';
} else {
return '2B FAILED';
}
string ask(let('len',200),if(call(get('=='),get('len'),200),block(return('2B PASSED')),block(if(call(get('=='),get('len'),10),block(return('2B FAILED')),block(list(return('2B FAILED')))))))
'2B PASSED'
🦄

test 2C

###test 3A
let len = 333
if(len == 3) {
return '3A FAILED';
} else if(len == 10) {
return '3A FAILED';
} else if(len == 333) {
return '3A PASSED';
} else {
return '3A FAILED';
}
string ask(let('len',333),if(call(get('=='),get('len'),3),block(return('3A FAILED')),block(if(call(get('=='),get('len'),10),block(return('3A FAILED')),block(if(call(get('=='),get('len'),333),block(return('3A PASSED')),block(list(return('3A FAILED')))))))))
'3A PASSED'
🦄

###test 3B
let len = 555
if(len == 555) {
return '3B PASSED';
} else if(len == 10) {
return '3B FAILED';
} else if(len == 65) {
return '3B FAILED';
} else {
return '3B FAILED';
}
string ask(let('len',555),if(call(get('=='),get('len'),555),block(return('3B PASSED')),block(if(call(get('=='),get('len'),10),block(return('3B FAILED')),block(if(call(get('=='),get('len'),65),block(return('3B FAILED')),block(list(return('3B FAILED')))))))))
'3B PASSED'
🦄

###test 3C
let len = 12345
if(len == 555) {
return '3C FAILED';
} else if(len == 10) {
return '3C FAILED';
} else if(len == 12345) {
return '3C PASSED';
} else {
return '3C FAILED';
}
string ask(let('len',12345),if(call(get('=='),get('len'),555),block(return('3C FAILED')),block(if(call(get('=='),get('len'),10),block(return('3C FAILED')),block(if(call(get('=='),get('len'),12345),block(return('3C PASSED')),block(list(return('3C FAILED')))))))))
'3C PASSED'
🦄

###test 3D
let len = 99999
if(len == 555) {
return '3D FAILED';
} else if(len == 10) {
return '3D FAILED';
} else if(len == 12345) {
return '3D FAILED';
} else {
return '3D PASSED';
}
string ask(let('len',99999),if(call(get('=='),get('len'),555),block(return('3D FAILED')),block(if(call(get('=='),get('len'),10),block(return('3D FAILED')),block(if(call(get('=='),get('len'),12345),block(return('3D FAILED')),block(list(return('3D PASSED')))))))))
'3D PASSED'
🦄

npm run build
Test Suites: 4 passed, 4 total
Tests: 4 passed, 4 total
Snapshots: 0 total
Time: 3.689 s
Ran all test suites.

npm test
Test Suites: 272 passed, 272 total
Tests: 27 todo, 725 passed, 752 total
Snapshots: 0 total
Time: 20.313 s, estimated 29 s
Ran all test suites.

czerwinskilukasz1 · 2020-08-11T08:51:48Z

@markkulube , great to see that it worked.
The test cases look fine, I see you are testing each branch of if-else if-else construct, good.
One more thing is that the tests should be placed in src/askscript/__tests__/04-control-flow folder and should be checking the result automatically. There are several ways to do it. I'd recommend writing an .ask file and a corresponding .test.result.ts file (like src/askscript/tests/04-control-flow/program10b-if_else.ask and src/askscript/tests/04-control-flow/program10b-if_else.test.result.ts) for each of the cases you want to test.

Please submit your PR and mark me as a reviewer.

czerwinskilukasz1 · 2020-08-11T08:55:57Z

Ah, some more nit comments for your tests:

it would be great if you tested branches of an if-else if-else construct in order - 1st branch in the 1st test, 2nd branch in the 2nd one etc., so test 1B and 1A should be swapped, tests 2 go as: 2B 2A 2C and tests 3 go in a different order too.
connected to the bullet above, I think test 3A and 3C are testing the same branch. When they are written in order, it's much easier to spot it.

markkulube · 2020-08-12T05:50:12Z

@czerwinskilukasz1 thanks for the feedback. Let me get 1 basic test going in the AskQL test suite first, then i will gauge if/which others to include.

An exception is thrown after including tests scripts to the control flow tests from folder. Looks like it has to do with formatting but i ran "Format Document" in VSCode using the Prettier plugin. Kinda stuck.

src/askscript/tests/04-control-flow/program10d-if_else_if_else_1a.ask:

ask {
len = 10
if(len == 2) {
return '1A FAILED';
} else if(len == 10) {
return '1A PASSED';
} else {
return '1A FAILED';
}
}

src/askscript/tests/04-control-flow/program10d-if_else_if_else_1a.test.result.ts:

export = '1A PASSED';

npm test
Summary of all failing tests
FAIL src/askscript/tests/04-control-flow/program10d-if_else_if_else_1a.ask
● Test suite failed to run

**TypeError: Cannot read property 'type' of undefined**

  125 |   });
  126 |
> 127 |   const askFormatted = prettier.format(source, {
      |                                 ^
  128 |     parser: 'askscript' as prettier.BuiltInParserName,
  129 |     plugins: [prettierPluginAskScript],
  130 |     // TODO load options from file

  at propagateBreaksOnEnterFn (node_modules/prettier/index.js:13499:13)
  at traverseDoc (node_modules/prettier/index.js:13359:11)
  at Object.propagateBreaks (node_modules/prettier/index.js:13524:3)
  at printAstToDoc (node_modules/prettier/index.js:14541:14)
  at coreFormat (node_modules/prettier/index.js:14799:15)
  at format (node_modules/prettier/index.js:15019:75)
  at formatWithCursor (node_modules/prettier/index.js:15035:12)
  at node_modules/prettier/index.js:51620:12
  at Object.format (node_modules/prettier/index.js:51640:12)
  at src/test.jest.testRunner.ts:127:33

Test Suites: 1 failed, 272 passed, 273 total
Tests: 27 todo, 725 passed, 752 total
Snapshots: 0 total
Time: 30.222 s, estimated 43 s
Ran all test suites.
npm ERR! Test failed. See above for more details.

markkulube · 2020-08-19T19:34:07Z

@czerwinskilukasz1 @mhagmajer the sooner i get help with the test script, the sooner we can close this PR, and hopefully integrate if-else if-else support to AskQL.

should i just submitt a draft PR with the buggy test case for if-else if-else, and maybe you can better help me that way?

mhagmajer · 2020-08-19T19:51:41Z

@markkulube yes, I think such PR would be helpful to drive the matter forward 🙂 Thanks for continued interest in solving this issue, it's appreciated 👍

markkulube · 2020-08-19T21:50:42Z

@czerwinskilukasz1 @mhagmajer thanks, i really want to see this through especially because i have else-if else-if working in my local env.

the draft PR has been raised.

both of you should be able to edit.

looking for further progress with this.

czerwinskilukasz1 added AskScript ./src/askscript/** discussion A discussion is taking place, please don't work on it yet enhancement New feature or request type:syntax related to syntax or syntax sugar labels Jul 9, 2020

czerwinskilukasz1 removed the discussion A discussion is taking place, please don't work on it yet label Jul 9, 2020

czerwinskilukasz1 changed the title ~~How about we allow 'else if'?~~ Allow 'else if' construct Jul 9, 2020

czerwinskilukasz1 added this to the v1.4 milestone Jul 9, 2020

czerwinskilukasz1 added the first-timers-only label Jul 10, 2020

czerwinskilukasz1 mentioned this issue Jul 10, 2020

2<ws>+<ws>3 does not parse #30

Closed

czerwinskilukasz1 assigned markkulube Jul 13, 2020

czerwinskilukasz1 mentioned this issue Aug 10, 2020

feat(cli): add welcome message #446

Merged

markkulube mentioned this issue Aug 19, 2020

feat(askscript): allow else if construct #459

Merged

czerwinskilukasz1 closed this as completed in #459 Oct 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow 'else if' construct #378

Allow 'else if' construct #378

czerwinskilukasz1 commented Jul 9, 2020

czerwinskilukasz1 commented Jul 9, 2020

mhagmajer commented Jul 9, 2020

czerwinskilukasz1 commented Jul 9, 2020

czerwinskilukasz1 commented Jul 9, 2020

czerwinskilukasz1 commented Jul 9, 2020

markkulube commented Jul 10, 2020

czerwinskilukasz1 commented Jul 13, 2020

czerwinskilukasz1 commented Jul 13, 2020

markkulube commented Jul 15, 2020

czerwinskilukasz1 commented Jul 15, 2020

czerwinskilukasz1 commented Jul 30, 2020 •

edited

Loading

markkulube commented Jul 30, 2020 •

edited

Loading

czerwinskilukasz1 commented Jul 30, 2020

markkulube commented Aug 2, 2020 •

edited

Loading

markkulube commented Aug 2, 2020 •

edited

Loading

czerwinskilukasz1 commented Aug 3, 2020

czerwinskilukasz1 commented Aug 3, 2020

markkulube commented Aug 4, 2020

czerwinskilukasz1 commented Aug 4, 2020

markkulube commented Aug 5, 2020 •

edited

Loading

czerwinskilukasz1 commented Aug 10, 2020

markkulube commented Aug 11, 2020 •

edited

Loading

czerwinskilukasz1 commented Aug 11, 2020

czerwinskilukasz1 commented Aug 11, 2020

markkulube commented Aug 12, 2020 •

edited

Loading

markkulube commented Aug 19, 2020

mhagmajer commented Aug 19, 2020

markkulube commented Aug 19, 2020

Allow 'else if' construct #378

Allow 'else if' construct #378

Comments

czerwinskilukasz1 commented Jul 9, 2020

czerwinskilukasz1 commented Jul 9, 2020

mhagmajer commented Jul 9, 2020

czerwinskilukasz1 commented Jul 9, 2020

czerwinskilukasz1 commented Jul 9, 2020

czerwinskilukasz1 commented Jul 9, 2020

markkulube commented Jul 10, 2020

czerwinskilukasz1 commented Jul 13, 2020

czerwinskilukasz1 commented Jul 13, 2020

markkulube commented Jul 15, 2020

czerwinskilukasz1 commented Jul 15, 2020

czerwinskilukasz1 commented Jul 30, 2020 • edited Loading

markkulube commented Jul 30, 2020 • edited Loading

czerwinskilukasz1 commented Jul 30, 2020

markkulube commented Aug 2, 2020 • edited Loading

markkulube commented Aug 2, 2020 • edited Loading

czerwinskilukasz1 commented Aug 3, 2020

czerwinskilukasz1 commented Aug 3, 2020

markkulube commented Aug 4, 2020

czerwinskilukasz1 commented Aug 4, 2020

markkulube commented Aug 5, 2020 • edited Loading

czerwinskilukasz1 commented Aug 10, 2020

markkulube commented Aug 11, 2020 • edited Loading

test 2C

czerwinskilukasz1 commented Aug 11, 2020

czerwinskilukasz1 commented Aug 11, 2020

markkulube commented Aug 12, 2020 • edited Loading

markkulube commented Aug 19, 2020

mhagmajer commented Aug 19, 2020

markkulube commented Aug 19, 2020

czerwinskilukasz1 commented Jul 30, 2020 •

edited

Loading

markkulube commented Jul 30, 2020 •

edited

Loading

markkulube commented Aug 2, 2020 •

edited

Loading

markkulube commented Aug 2, 2020 •

edited

Loading

markkulube commented Aug 5, 2020 •

edited

Loading

markkulube commented Aug 11, 2020 •

edited

Loading

markkulube commented Aug 12, 2020 •

edited

Loading