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(let): refactor let #526

Merged

Conversation

dastin-sandura
Copy link
Contributor

@dastin-sandura dastin-sandura commented Oct 27, 2020

closes #521

add questions about the details and importance of this feature request
…late

Update feature request issue template
@dastin-sandura
Copy link
Contributor Author

Hi @YonatanKra , could you please review my changes in this PR? 🙂

feat(chore): feature request issue template (CatchTheTornado#525)
Copy link
Contributor

@mhagmajer mhagmajer left a comment

Choose a reason for hiding this comment

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

Good progress @dastin-sandura, thanks for working on this 👍

add file with implementation of const, remove const parts from let.ts
extract const and assign from let.ts, extract tests for const and assign
from let tests
@dastin-sandura
Copy link
Contributor Author

Thank you for your review 😄 I will review your comments and make changes

.github/ISSUE_TEMPLATE/feature_request.md Outdated Show resolved Hide resolved
src/askvm/resources/core/assign.ts Outdated Show resolved Hide resolved
src/askvm/resources/core/assign.ts Show resolved Hide resolved
src/askvm/resources/core/assign.ts Show resolved Hide resolved
src/askvm/resources/core/const.ts Show resolved Hide resolved
@dastin-sandura
Copy link
Contributor Author

Thank you for your reviews 😄 @mhagmajer and @YonatanKra
@YonatanKra I have implemented almost all the changes you have requested. I did not extract the logic for checking the reserved keyword as I want to secure this PR 🙂 I will take care of extracting this in the next Github issue.

Copy link
Collaborator

@YonatanKra YonatanKra left a comment

Choose a reason for hiding this comment

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

Great job! Almost there!
See my comments. The only must is the change in the tests (to use an expect syntax).
The rest are mostly clarifications I could use.

src/askvm/resources/core/__test__/const.test.ts Outdated Show resolved Hide resolved
src/askvm/resources/core/assign.ts Show resolved Hide resolved
src/askvm/resources/core/assign.ts Show resolved Hide resolved
Copy link
Collaborator

@YonatanKra YonatanKra left a comment

Choose a reason for hiding this comment

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

LGTM. @mhagmajer ?

@mhagmajer mhagmajer merged commit bbba138 into CatchTheTornado:master Oct 30, 2020
km4 pushed a commit to km4/askql that referenced this pull request Nov 28, 2020
* Default issue template, proposed by Github

* Update feature request issue template

add questions about the details and importance of this feature request

* Changes

add file with implementation of const, remove const parts from let.ts
extract const and assign from let.ts, extract tests for const and assign
from let tests

* remove logs, unused function, and check if code === const

* change 'let' test retrieval of a let variable value instead of checking what let returns

* remove code which runs when using 'assign' from let.ts

* remove condition on assign, add flag to decide when to throw 'unknown variable error'

* remove commented code from assign.ts

* Test for 'assign' which makes sure that you cannot assign value to a constant after it was initialized

* Change test naming

* const.test for making sure that const is Object.isFrozen()

* assign test for throwing when assigning value to an undeclared variable

* assign test for checking assignment to reserved keyword variables

* change if to proper jest expect assertion
km4 added a commit to km4/askql that referenced this pull request Nov 28, 2020
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.

Refactor let
3 participants