Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Consistent string and var for steps echo, dump, write, check #693

Merged
merged 3 commits into from
Feb 17, 2020

Conversation

kensoh
Copy link
Member

@kensoh kensoh commented Jan 19, 2020

@siowyisheng raising draft PR for your review and testing. This change is quite pervasive, will need to work together on some iterations to cover edge cases thoroughly.

@siowyisheng
Copy link
Contributor

to continue to test separately

@siowyisheng
Copy link
Contributor

Edge cases include:

  1. user wants to echo single quote

@kensoh
Copy link
Member Author

kensoh commented Jan 20, 2020

echo 123'456 doesn't work but we don't really expect users to escape since this is not enclosed within quotation marks. Need to explore more edge cases like these and fix.

@kensoh
Copy link
Member Author

kensoh commented Jan 27, 2020

@siowyisheng initial finding turns out the behaviour with single quote is consistent. echo 123'456 throws error just as type q as 123'456. Using double quote works for both and using \' works for both. If this is to be fixed, then the behaviour for other steps should also be fixed.

@kensoh
Copy link
Member Author

kensoh commented Jan 28, 2020

Hi @siowyisheng this looks good after doing some tests. For your checks.

1 potential improvement is whether to make single quote usable without the escape character, though this is the same behaviour for all steps. So we can evaluate that separately if that makes sense.

I thought over whether this change can be made backward compatible by detecting input passed in from the script. It seems like it isn't possible to do it deterministically. There isn't a fool proof way to detect for sure whether user is typing quotes to mean string or really want to echo a quotation mark. Similarly for '+variable+' structure. If use regex or some matching of this form to 'intelligently' recognise old format, what if user really wants to echo this string as a string.

This seems like a change that cant be made backward compatible.

Another idea is we can evaluate all those changes in v6 and create some migration script to convert old scripts to new scripts format (eg .tag extension, formatting used by echo, dump, write, check). This could remove user friction substantially for scripts that uses these steps a lot.

@kensoh kensoh requested a review from siowyisheng January 28, 2020 02:16
@kensoh
Copy link
Member Author

kensoh commented Feb 6, 2020

Was reading your new docs and saw this.. It seems like for conditions and defining of variables there will still be 'inconsistency' as text needs to be enclosed in quotation marks and variables with backticks. Hmm... Can't really remove the need for quotations marks is that correct? For eg below, if remove quotation marks then it can introduce ambiguous situations to tell which is operator which is text.

if url() contains "success"
{
  click button1.png
  click button2.png
}

@kensoh
Copy link
Member Author

kensoh commented Feb 7, 2020

As discussed, implementing as planned. Least of all evils.

@siowyisheng siowyisheng merged commit c3d22a4 into develop Feb 17, 2020
@siowyisheng
Copy link
Contributor

tested and LGTM

1 similar comment
@siowyisheng
Copy link
Contributor

tested and LGTM

@siowyisheng siowyisheng deleted the consistent_str_var branch February 17, 2020 09:40
siowyisheng pushed a commit that referenced this pull request Mar 10, 2020
* consistency in string and var context

* consistency for string and var in steps

* test signature for change
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants