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

Mistakable error message: Variable not available #186

Closed
HolQue opened this issue Jan 9, 2024 · 14 comments
Closed

Mistakable error message: Variable not available #186

HolQue opened this issue Jan 9, 2024 · 14 comments

Comments

@HolQue
Copy link
Collaborator

HolQue commented Jan 9, 2024

The JSON code:

"intval" : 1,
"testlist" : ["B", 2],
"param_${testlist}['${intval}']}" : 3

causes:

Error: 'The variable '${testlist}['1']' is not available!'!

Here I have doubts about the meaning of the single quotes inside the square brackets. Are they allowed/required or not? The requirements are unclear to me.

But I expect that expressions are resolved from innermost to outermost. What should be the order of computation in the code example above?

I expect:

  1. Detect parameter intval
  2. Parameter intval does exist
  3. Parameter intval is of type int
  4. Parameter intval is wrapped in single quotes, therefore will be interpreted as string
  5. This is found inside square brackets, therefore can either be an index or a key name
  6. The parameter, the square bracket expression belongs to, is of type list, therefore the content of the square brackets is expected to be of type int (but is of type str, and this is an error).

In my opinion it should be possible to provide a corresponding error message, telling that list indices are expected to be of type int.

To tell only that a variable does not exist, does not really help the users.

And independent from this: Quotes should not be accepted as part of parameter names (or dictionary key names). Seems that the JsonPreprocessor still has a common issue with proper handling of quotes inside expressions.

Reference: JPP_0268

@test-fullautomation
Copy link
Owner

test-fullautomation commented Jan 11, 2024

Hi Son,

a possible speaking error messge would be:
${testlist} expects integer as index. Got string instead in 'param_${testlist}['${intval}']}'.

if above is not possible, then a generic message could be:
Expression '"param_${testlist}['${intval}']}" : 3' can't be evaluated.

Thank you,
Thomas

@test-fullautomation
Copy link
Owner

Hi Son,
what is the status of this ticket?
Thank you,
Thomas

@namsonx
Copy link
Collaborator

namsonx commented Jan 31, 2024

Hello Thomas,

I will implement this ticket in next 0.11.0 release.

Thank you,
Son

@test-fullautomation test-fullautomation modified the milestones: 0.10.0, 0.11.0 Mar 5, 2024
@HolQue
Copy link
Collaborator Author

HolQue commented Mar 26, 2024

Update:

The following line from code example above, is invalid in between:

"param_${testlist}['${intval}']}" : 3

Because of the decision, not to create parameter names dynamically.

Therefore the code example needs to be adapted:

"intval" : 1,
"testlist" : ["B", 2],
"param" : ${testlist}['${intval}']

Result:

Error: 'The variable '${testlist}['1']' is not available!'!

That's wrong. This is still a type issue, and not a missing variable issue.

Expected error message: List indices have to be of type int.

@namsonx
Copy link
Collaborator

namsonx commented Apr 8, 2024

Hello Thomas,

I implemented this ticket and pushed the change to stabi branch.

Thank you,
Son

@HolQue
Copy link
Collaborator Author

HolQue commented May 7, 2024

"intval" : 1,
"testlist" : ["B", 2],
"param" : ${testlist}['${intval}']

causes:

Error: '${testlist} expects integer as index. Got string instead in ${testlist}['${intval}']'!

This is like expected now.

But the same with a string parameter:

"strval" : "ABC",
"testlist" : ["B", 2],
"param" : ${testlist}[${strval}]

causes:

Error: 'The parameter '${testlist}['ABC']' is not available!'!

But expected is the same error message like above.

@namsonx
Copy link
Collaborator

namsonx commented May 16, 2024

Hello Holger,

Thank you for your finding! I'm going to forward the exception from Python the error message for both cases like below:
Exception: Could not resolve expression '${testlist}['1']'. Reason: list indices must be integers, not str
Exception: Could not resolve expression '${testlist}['ABC']'. Reason: list indices must be integers, not str
If the new error message is fine, I will push the change to stabi branch. Please give me your feedback?

Thank you,
Son

@HolQue
Copy link
Collaborator Author

HolQue commented May 16, 2024

Hi Son,

that's fine to me. And you should adapt the wording a little bit.

Either:

list indices must be of type 'int', not 'str'

or:

list indices must be integers, not strings

@HolQue
Copy link
Collaborator Author

HolQue commented May 16, 2024

Hi Son,

see also #306

Should be aligned.

@namsonx
Copy link
Collaborator

namsonx commented May 16, 2024

Hi Son,

that's fine to me. And you should adapt the wording a little bit.

Either:

list indices must be of type 'int', not 'str'

or:

list indices must be integers, not strings

Hello Holger,

The reason list indices must be integers, not str is forwarded from Python's exception.
Do we need to modify the exception message from Python?

Thank you,
Son

@HolQue
Copy link
Collaborator Author

HolQue commented May 16, 2024

Hi Son,

oops - I was not aware of that. No modification please.

@namsonx
Copy link
Collaborator

namsonx commented May 16, 2024

Hello Holger,

I push commit f3df857af39 to solve your review comment.

Thank you,
Son

@HolQue
Copy link
Collaborator Author

HolQue commented May 31, 2024

Retest successful. Issue can be closed.

@test-fullautomation
Copy link
Owner

integrated in 0.12.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

3 participants