Skip to content

Conversation

@namsonx
Copy link
Collaborator

@namsonx namsonx commented Sep 28, 2022

No description provided.

@test-fullautomation
Copy link
Owner

test-fullautomation commented Sep 28, 2022

Hi Son,
thank you for you pull-request.
This I will merge for coming 0.5.3 version.
Please provide also selftest and documentation update.
Thank you,
Thomas

Copy link
Collaborator

@HolQue HolQue left a comment

Choose a reason for hiding this comment

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

If the key value is of type string it makes no difference if the complete expression is encapsulated in quotes or not:
(1) "${params}['global']['string_val']"
(2) ${params}['global']['string_val']
This is OK, but the user should be aware of that - therefore this should be mentioned anywhere in the documentation, maybe as recommendation that the quotes are not necessary in this case.

Keys of type int, float and bool are recognized correctly (when expression is without quotes). With quotes the values are of type string correctly.

Keys that are None or null are also recognized correctly (but independently from quotes around the expression).

Also with quotes ("${params}['global']['null_val']") the value is of type NoneType. But in my opinion it would be more consistent when with quotes the type would be string.

@test-fullautomation
Copy link
Owner

Hi Holger,
thank you for fast testing!
Yes, this you found an issue. null and None must be string (text) if they are set in quotes.
@namsonx : Please fix this.
Thank you,
Thomas

@namsonx
Copy link
Collaborator Author

namsonx commented Sep 29, 2022

Hello Thomas,

 I have just run the test with debug mode, and I observed the value of "${params}['global']['null_val']" is converted to string and remained until it is set by BuiltIn().set_global_variable("${%s}" % k.strip(), v)
 I could not attach the screenshot of my debug session to this github comment, I will show you in the sync up meeting.
 So, the root cause of the case which Holger mentioned comes from the core of ROBFW, I think!

Thank you,
Son

@test-fullautomation test-fullautomation added this to the 0.5.3.0 Win10 milestone Oct 17, 2022
@test-fullautomation test-fullautomation added enhancement New feature or request 0.6.0 and removed 0.5.3 labels Nov 4, 2022
@test-fullautomation
Copy link
Owner

Hi Son,
unfortunately the SelfTest and Documentation update is still missing.
By when can I expect this?
We disussed a lot on this one month ago. Is there something remaining open / or a dependency? What about the "None" issue?
Thank you,
Thomas

@namsonx
Copy link
Collaborator Author

namsonx commented Nov 7, 2022

Hello Thomas,

 I had checked the case with "None" issue which Holger mentioned, and I recognized that this issue also occurs with "True" and "False" cases.
 I had updated the commit to correct them all in this pull request.

 For the Selftest documentation of this JsonPreprocessor repo. I will update in another pull-request.

Thank you,
Son

@test-fullautomation
Copy link
Owner

Hi Son,
unfortunately the SelfTest and Documentation update is still missing.
By when can I expect this?
Thank you,
Thomas

@test-fullautomation
Copy link
Owner

Hi Son,
thank you for the update.
Please look into my findings.
Thank you,
Thomas

Copy link
Owner

@test-fullautomation test-fullautomation left a comment

Choose a reason for hiding this comment

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

Hi Son,
look O.K. for me.
Let us test. I will do further maintenance of the documentation.
Thank you,
Thomas

--------------

.. image:: ./pictures/python3-jsonpreprocessor.png
Firstly, clone `python-jsonpreprocessor <https://github.com/test-fullautomation/python-jsonpreprocessor>`_

Choose a reason for hiding this comment

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

Hi Son,
pease refer to genpackagedoc. I sent some weeks ago a mail that genpackagedoc shall be the reference.
first we propose pip install.
Please make this document similar.
Thank you,
Thomas

Choose a reason for hiding this comment

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

Hi Son,
I will maintain this.
Thank you,
Thomas

@test-fullautomation test-fullautomation merged commit 2b00970 into develop Dec 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

0.6.0 enhancement New feature or request

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants