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

implement mod resource (% operator) #250

Closed
czerwinskilukasz1 opened this issue Jun 27, 2020 · 15 comments · Fixed by #437
Closed

implement mod resource (% operator) #250

czerwinskilukasz1 opened this issue Jun 27, 2020 · 15 comments · Fixed by #437
Assignees
Labels
AskVM ./src/askvm/** enhancement New feature or request first-timers-only good first issue Good for newcomers help wanted Extra attention is needed type:operators related to operators type:resources related to AskVM resources
Milestone

Comments

@czerwinskilukasz1
Copy link
Collaborator

czerwinskilukasz1 commented Jun 27, 2020

If we have division and multiplication operators, we should also have a modulo operator. However, currently it does not exist.

🦄 40/6
float ask(call(get('/'),40,6))
6.666666666666667
🦄 40*6
int ask(call(get('*'),40,6))
240
🦄 .editor
// Entering editor mode (^D to finish, ^C to cancel)
40%6

Uncaught Error: Unknown identifier '%'!
    at Object.<anonymous> (/Users/milimetr/Desktop/x/askql/dist/askvm/lib/run.js:70:15)
    at Generator.next (<anonymous>)
    at /Users/milimetr/Desktop/x/askql/dist/askvm/lib/run.js:8:71
    at new Promise (<anonymous>)
    at __awaiter (/Users/milimetr/Desktop/x/askql/dist/askvm/lib/run.js:4:12)
    at Object.run (/Users/milimetr/Desktop/x/askql/dist/askvm/lib/run.js:29:12)
🦄

This task is to create a mod resource and a % operator linked to this resource.

--

Dear new contributor,

If you have any questions on how to start, please write a comment with a tag: @czerwinskilukasz1 or @mhagmajer, or join our Discord Community and let us know there. We will be very happy to help you start! :)
Hint for starters: As a starting point, take a look at src/askvm/resources/math/index.ts file, where a * operator and times resource are already linked or a couple of other ones and use them as an example.

Cheers,
Łukasz (@czerwinskilukasz1)
AskQL Core Developer

@czerwinskilukasz1 czerwinskilukasz1 added enhancement New feature or request good first issue Good for newcomers AskVM ./src/askvm/** labels Jun 27, 2020
@mhagmajer
Copy link
Contributor

Are we sure that we want to add an operator for modulo? We could just have mod function or a mod b expression. I'm asking because % already has meaning associated to it by math.

@czerwinskilukasz1
Copy link
Collaborator Author

As for %, I'd mimic Javascript to be honest.

a mod b expression looks good, how about we allow to write: a <funName> b for any function name ?
So that people could write also: 'a' concat 'b' or 1 plus 3? It looks to me like Scala and I like it.

@mhagmajer
Copy link
Contributor

@czerwinskilukasz1 we actually already have a:concat(b) and do what you described for operators. Let's maybe keep one way of doing things so people don't need to make choice when writing AskScript programs.

@czerwinskilukasz1
Copy link
Collaborator Author

@mhagmajer , so I wouldn't introduce a new syntax a mod b for just one operation.

@mhagmajer
Copy link
Contributor

@czerwinskilukasz1 sure, that would make AskScript easier for mathematicians which I think is an advantage.

So how about using % for percentage? I don't have strong opinion on this, however, just the fact that JavaScript uses this for modulo is not a sufficient argument for me here

@mhagmajer
Copy link
Contributor

@czerwinskilukasz1 also how about a div b so integer division?

@czerwinskilukasz1
Copy link
Collaborator Author

czerwinskilukasz1 commented Jun 27, 2020

@czerwinskilukasz1 sure, that would make AskScript easier for mathematicians which I think is an advantage.

So how about using % for percentage? I don't have strong opinion on this, however, just the fact that JavaScript uses this for modulo is not a sufficient argument for me here

I can't think of a better operator. Also, at least 20 other programming languages, including C/C++, C#, Java, Python, PHP and Kotlin, are using % for modulo.

As for percent, I'm totally not sure how we could use it.

@czerwinskilukasz1 also how about a div b so integer division?

So for what class of operations you would use this syntax?

@mhagmajer
Copy link
Contributor

mhagmajer commented Jun 27, 2020

@czerwinskilukasz1

7 = ((7 div 3) * 3) + (7 mod 3)

About % we could say

let price = 1000 * 20%

I guess this would only make sense as a unary operator so the binary can be used for module. Would it be possible for the parser to support both?

@czerwinskilukasz1
Copy link
Collaborator Author

@czerwinskilukasz1

7 = ((7 div 3) * 3) + (7 mod 3)

I see, I see. Looks pretty good.
So, would we just hardcode it into AskScript grammar for just div and mod or rather for all identifiers?

About % we could say

let price = 1000 * 20%

I'm not convinced to this.

@mhagmajer
Copy link
Contributor

@czerwinskilukasz1 let's do it in parser only for operators, div and mod and carry the discussion about unary % on a separate issue

@czerwinskilukasz1
Copy link
Collaborator Author

%: #264

@czerwinskilukasz1 czerwinskilukasz1 added the type:operators related to operators label Jul 1, 2020
@czerwinskilukasz1 czerwinskilukasz1 added help wanted Extra attention is needed type:resources related to AskVM resources P2 first-timers-only and removed P2 labels Jul 8, 2020
@czerwinskilukasz1
Copy link
Collaborator Author

As for a mod b and a div b syntax, I moved it to issue #411

@czerwinskilukasz1 czerwinskilukasz1 changed the title implement modulo resource (% operator) implement mod resource (% operator) Jul 26, 2020
@czerwinskilukasz1
Copy link
Collaborator Author

@develper , following your comment, this issue is yours :)
If you had any questions, just tag me here.

bhargav-khalasi added a commit to bhargav-khalasi/askql that referenced this issue Aug 1, 2020
@bhargav-khalasi
Copy link
Contributor

@czerwinskilukasz1 , I have implemented the mod resource and want to know should I write tests for the same? If yes, then which test cases I should include.

@czerwinskilukasz1
Copy link
Collaborator Author

@czerwinskilukasz1 , I have implemented the mod resource and want to know should I write tests for the same? If yes, then which test cases I should include.

Great.

Let's add an example of <int> % <int> whose result should be 0, an example of <int> % <int> whose result should be non-zero, an example of <float> % <int> and <float> % <float>.
The same tests should be written twice for AskScript - once for mod() resource and once for % operator.
See src/askscript/__tests__/09-operators/operators-09-logicalOr-oper.test.ts and src/askscript/__tests__/09-operators/operators-09-logicalOr-res.test.ts for an example of such tests.

bhargav-khalasi added a commit to bhargav-khalasi/askql that referenced this issue Aug 4, 2020
czerwinskilukasz1 pushed a commit that referenced this issue Aug 10, 2020
* fix: create OS agnostic build command/s for rm and mkdir (#395)

* feat: implement mod resource(#250)

* feat: implement mod resource (#250)
markkulube pushed a commit to markkulube/askql that referenced this issue Sep 13, 2020
* fix: create OS agnostic build command/s for rm and mkdir (CatchTheTornado#395)

* feat: implement mod resource(CatchTheTornado#250)

* feat: implement mod resource (CatchTheTornado#250)
czerwinskilukasz1 added a commit that referenced this issue Oct 7, 2020
* build(deps-dev): bump @jest/environment from 26.1.0 to 26.2.0 (#425)

Bumps [@jest/environment](https://github.com/facebook/jest/tree/HEAD/packages/jest-environment) from 26.1.0 to 26.2.0.
- [Release notes](https://github.com/facebook/jest/releases)
- [Changelog](https://github.com/facebook/jest/blob/master/CHANGELOG.md)
- [Commits](https://github.com/facebook/jest/commits/v26.2.0/packages/jest-environment)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps-dev): bump expect from 26.1.0 to 26.2.0 (#427)

Bumps [expect](https://github.com/facebook/jest/tree/HEAD/packages/expect) from 26.1.0 to 26.2.0.
- [Release notes](https://github.com/facebook/jest/releases)
- [Changelog](https://github.com/facebook/jest/blob/master/CHANGELOG.md)
- [Commits](https://github.com/facebook/jest/commits/v26.2.0/packages/expect)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps-dev): bump @types/node from 14.0.26 to 14.0.27 (#429)

Bumps [@types/node](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/node) from 14.0.26 to 14.0.27.
- [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases)
- [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/node)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps-dev): bump nanoid from 3.1.10 to 3.1.12 (#430)

Bumps [nanoid](https://github.com/ai/nanoid) from 3.1.10 to 3.1.12.
- [Release notes](https://github.com/ai/nanoid/releases)
- [Changelog](https://github.com/ai/nanoid/blob/master/CHANGELOG.md)
- [Commits](ai/nanoid@3.1.10...3.1.12)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps-dev): bump webpack from 4.44.0 to 4.44.1 (#432)

Bumps [webpack](https://github.com/webpack/webpack) from 4.44.0 to 4.44.1.
- [Release notes](https://github.com/webpack/webpack/releases)
- [Commits](webpack/webpack@v4.44.0...v4.44.1)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps-dev): bump @types/cors from 2.8.6 to 2.8.7 (#433)

Bumps [@types/cors](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/cors) from 2.8.6 to 2.8.7.
- [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases)
- [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/cors)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps-dev): bump @jest/types from 26.1.0 to 26.2.0 (#426)

Bumps [@jest/types](https://github.com/facebook/jest/tree/HEAD/packages/jest-types) from 26.1.0 to 26.2.0.
- [Release notes](https://github.com/facebook/jest/releases)
- [Changelog](https://github.com/facebook/jest/blob/master/CHANGELOG.md)
- [Commits](https://github.com/facebook/jest/commits/v26.2.0/packages/jest-types)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps-dev): bump @babel/preset-env from 7.10.4 to 7.11.0 (#428)

Bumps [@babel/preset-env](https://github.com/babel/babel/tree/HEAD/packages/babel-preset-env) from 7.10.4 to 7.11.0.
- [Release notes](https://github.com/babel/babel/releases)
- [Changelog](https://github.com/babel/babel/blob/main/CHANGELOG.md)
- [Commits](https://github.com/babel/babel/commits/v7.11.0/packages/babel-preset-env)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* chore: remove npm-watch (#438)

Co-authored-by: Karol Milewski <km4@pc50.home>

* build(deps-dev): bump jest from 26.1.0 to 26.2.2 (#431)

Bumps [jest](https://github.com/facebook/jest) from 26.1.0 to 26.2.2.
- [Release notes](https://github.com/facebook/jest/releases)
- [Changelog](https://github.com/facebook/jest/blob/master/CHANGELOG.md)
- [Commits](jestjs/jest@v26.1.0...v26.2.2)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* feat(askvm): toInt (#408)

* feat(askvm): toInt

* Update toInt.test.ts

* Updates toInt test

* Extends tests for toInt

Co-authored-by: Richard <ricmoran@ebay.com>

* feat: implement mod resource (#250) (#437)

* fix: create OS agnostic build command/s for rm and mkdir (#395)

* feat: implement mod resource(#250)

* feat: implement mod resource (#250)

* build(deps-dev): bump @typescript-eslint/eslint-plugin (#440)

Bumps [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/eslint-plugin) from 3.5.0 to 3.8.0.
- [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases)
- [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/CHANGELOG.md)
- [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v3.8.0/packages/eslint-plugin)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps-dev): bump @types/jest from 26.0.3 to 26.0.9 (#444)

Bumps [@types/jest](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/jest) from 26.0.3 to 26.0.9.
- [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases)
- [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/jest)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps-dev): bump @babel/core from 7.10.5 to 7.11.1 (#445)

Bumps [@babel/core](https://github.com/babel/babel/tree/HEAD/packages/babel-core) from 7.10.5 to 7.11.1.
- [Release notes](https://github.com/babel/babel/releases)
- [Changelog](https://github.com/babel/babel/blob/main/CHANGELOG.md)
- [Commits](https://github.com/babel/babel/commits/v7.11.1/packages/babel-core)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps-dev): bump eslint from 7.5.0 to 7.6.0 (#443)

Bumps [eslint](https://github.com/eslint/eslint) from 7.5.0 to 7.6.0.
- [Release notes](https://github.com/eslint/eslint/releases)
- [Changelog](https://github.com/eslint/eslint/blob/master/CHANGELOG.md)
- [Commits](eslint/eslint@v7.5.0...v7.6.0)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps-dev): bump @typescript-eslint/parser from 3.7.0 to 3.8.0 (#442)

Bumps [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/parser) from 3.7.0 to 3.8.0.
- [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases)
- [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/parser/CHANGELOG.md)
- [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v3.8.0/packages/parser)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps-dev): bump @types/lodash from 4.14.158 to 4.14.159 (#441)

Bumps [@types/lodash](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/lodash) from 4.14.158 to 4.14.159.
- [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases)
- [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/lodash)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* feat(cli): add welcome message (#446)

* feat(askscript): add grammer to support "else if" key word and eb== If | ElseIf

* feat(askscript): defined ElseIf statement and class.

* test: test script for else-if else-if control flow statement.

* build(deps-dev): bump babel-jest from 26.1.0 to 26.2.2 (#424)

Bumps [babel-jest](https://github.com/facebook/jest/tree/HEAD/packages/babel-jest) from 26.1.0 to 26.2.2.
- [Release notes](https://github.com/facebook/jest/releases)
- [Changelog](https://github.com/facebook/jest/blob/master/CHANGELOG.md)
- [Commits](https://github.com/facebook/jest/commits/v26.2.2/packages/babel-jest)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* feat(askvm): resolve merge conflicts

* style: run prettier

* style(prettier-plugin): concatenate if else block is instance of class Else.

* feat(askscript): boolean to specify if else block is instance of class Else.

* test: test different branch combinations in an `else-if else-if' ladder.

* style: run prettier.

* feat(askscript): better naming (props.elseType --> props.elseBlock)

* fix: linked-list traversal from root node to generate all branches of if-else block in *.formatted.ask and *.askcode.

* fix(askscript/_tests_): updated *.ast.ts arising from iimplentation to support `else-if` constuct.

* test: add missing *.askcode and *.ast.tsx for testing implementation of `else-if else-if` construct.

* fix: restore previous formatting.

* fix: restore to version with all required dependencies.

* fix: restore previous formatting.

* fix: restore previous formatting.

* fix: restore previous formatting.

* style:run prettier.

* test: added test for the case when `if else-if else` ladder terminates with else if branch.

* fix: variable redeclaration using let: var ladder --> let ladder.

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Karol Milewski <m1leskm4@gmail.com>
Co-authored-by: Karol Milewski <km4@pc50.home>
Co-authored-by: Richard Moran <richardmoran8@gmail.com>
Co-authored-by: Richard <ricmoran@ebay.com>
Co-authored-by: Bhargav Khalasi <39995785+develper@users.noreply.github.com>
Co-authored-by: Lukasz Czerwinski <czerwinskilukasz1@gmail.com>
Co-authored-by: markkulube <markkulube@users.noreply.github.com>
@czerwinskilukasz1 czerwinskilukasz1 added this to the v1.3 milestone Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AskVM ./src/askvm/** enhancement New feature or request first-timers-only good first issue Good for newcomers help wanted Extra attention is needed type:operators related to operators type:resources related to AskVM resources
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants