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.

Sign up for GitHub

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

Jump to bottom

at should throw if the key is missing #404

Closed
mhagmajer opened this issue Jul 17, 2020 · 7 comments · Fixed by #523
Closed

at should throw if the key is missing #404

mhagmajer opened this issue Jul 17, 2020 · 7 comments · Fixed by #523
Labels
AskVM ./src/askvm/** bug Something isn't working first-timers-only good first issue Good for newcomers

Comments

@mhagmajer
Copy link
Contributor 

ask {
    const marta = { firstName: 'Marta' }
    marta:at('firstName3')
}

returns null (empty) but an error was expected (playground with AskVM v. 1.2.0)
@mhagmajer mhagmajer added AskVM ./src/askvm/** bug Something isn't working first-timers-only good first issue Good for newcomers labels Jul 17, 2020
@mhagmajer
Copy link
Contributor Author

@czerwinskilukasz1 what's your opinion about this?

@mhagmajer
Copy link
Contributor Author

I'd really like not to have two empty values like in JavaScript

@czerwinskilukasz1
Copy link
Collaborator

I'm fine with throwing as soon as we have hasKey: #400

@czerwinskilukasz1
Copy link
Collaborator

hasKey is implemented, this is unblocked now.

@mhagmajer
Copy link
Contributor Author

Place to fix that in code: https://github.com/xFAANG/askql/blob/master/src/askvm/resources/core/at.ts

@dastin-sandura
Copy link
Contributor

dastin-sandura commented Oct 22, 2020 •
edited
Loading

I have created a pull request (https://github.com/xFAANG/askql/pull/523/files) with a unit test for this case. I will appreciate if someone could verify if the test is correct 😄
Next, I will work on changing the implementation of at to work as expected by the test

@YonatanKra YonatanKra mentioned this issue Oct 22, 2020
Merged
@dastin-sandura
Copy link
Contributor

My pull request is ready for review 😃

dastin-sandura pushed a commit to dastin-sandura/askql that referenced this issue Oct 24, 2020
issue CatchTheTornado#404
a267080 
 - test case for regular access of a list and object using 'at'
 - implementation of flow which detects our of bound error
   when using 'at' to access a lists element by its index
dastin-sandura pushed a commit to dastin-sandura/askql that referenced this issue Oct 24, 2020
issue CatchTheTornado#404
eff4ee5 
 - att test case and implementation to handle a case
   for accessing a negative index of a list
mhagmajer pushed a commit that referenced this issue Oct 28, 2020
@dastin-sandura
fix(at): throw error when using at to access undefined property (#523)
5a78dd6 
* unit test for case when using at to access property not defined before

* mend

* change message in the thrown error to match the unit test

* throw error only if accessing a non existing property of a non-null object

* tests for case when using list, implementation of throwing error when index is out of bounds

* issue #404

 - test case for regular access of a list and object using 'at'
 - implementation of flow which detects our of bound error
   when using 'at' to access a lists element by its index

* issue #404

 - att test case and implementation to handle a case
   for accessing a negative index of a list

* use template literals to format the error message more clearly

Co-authored-by: Dastin.Sandura <dastin.sandura@gft.com>
km4 pushed a commit to km4/askql that referenced this issue Nov 28, 2020
@dastin-sandura @km4
fix(at): throw error when using at to access undefined property (Catc…
5996c8d 
…hTheTornado#523)

* unit test for case when using at to access property not defined before

* mend

* change message in the thrown error to match the unit test

* throw error only if accessing a non existing property of a non-null object

* tests for case when using list, implementation of throwing error when index is out of bounds

* issue CatchTheTornado#404

 - test case for regular access of a list and object using 'at'
 - implementation of flow which detects our of bound error
   when using 'at' to access a lists element by its index

* issue CatchTheTornado#404

 - att test case and implementation to handle a case
   for accessing a negative index of a list

* use template literals to format the error message more clearly

Co-authored-by: Dastin.Sandura <dastin.sandura@gft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
AskVM ./src/askvm/** bug Something isn't working first-timers-only good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(at): throw error when using at to access undefined property dastin-sandura/askql
3 participants
@mhagmajer @dastin-sandura @czerwinskilukasz1