Apply validation rules for device preferences #134
Conversation
imurchie
left a comment
imurchie
left a comment
There was a problem hiding this comment.
I think at the top of this function there should be some logging to say that the device preferences are being validated. Otherwise when these errors come up they are kind of out of the blue.
| } | ||
|
|
||
| if (!_.isUndefined(prefs.SimulatorWindowCenter)) { | ||
| const verificationPattern = /\{\-?\d+(\.\d+)?,\-?\d+(\.\d+)?\}/; |
There was a problem hiding this comment.
Can you document this regex in some way? Perhaps in https://regex101.com/ or something?
There was a problem hiding this comment.
would it be ok https://regex101.com/r/2ZXOij/1 ?
There was a problem hiding this comment.
Awesome!
This points out, though, that the capture groups are unnecessary, since nothing is being done with what is captured (/\{\-?\d+\.\d+?,\-?\d+\.\d+?\}/ would suffice). I don't think it matters here because the efficiency is not really at issue for such a short search. But in general capturing text is much less performant in a regex.
There was a problem hiding this comment.
I just wanted to make sure the expression like this is not possible
{1.,2.}
Would it be possible without groups?
There was a problem hiding this comment.
Ah. Yes, you need them for that. Can you put the various variations in the linked regex test (you'll need to add the m flag so it is multiline there?
# should match
{100.0,100.0}
{100,100}
{100.0,100}
# should not match
{100.0, 100.0}
{100.0,100.}
There was a problem hiding this comment.
I thought a unit test would be enough for this purpose. But yeah, it's easy to add
| if (!_.isUndefined(prefs.SimulatorWindowCenter)) { | ||
| const verificationPattern = /\{\-?\d+(\.\d+)?,\-?\d+(\.\d+)?\}/; | ||
| if (!_.isString(prefs.SimulatorWindowCenter) || !verificationPattern.test(prefs.SimulatorWindowCenter)) { | ||
| log.errorAndThrow(`SimulatorWindowCenter is expected to match "{floatXPosition,floatYPosition}" format (without spaces). ` + |
There was a problem hiding this comment.
Why not accommodate spaces?
There was a problem hiding this comment.
because there are no spaces in the original plist, so I'm not sure if it works properly if I add some
There was a problem hiding this comment.
Ok. It might be good to allow them as input (even if the spaces need to be stripped out on the server side) since some language's serialization library might include spaces at some point. Perhaps that is better left for if/when such an event occurs. From the error message it would be obvious what the problem is.
There was a problem hiding this comment.
I expect the value to be passed as a simple string, so nothing is going to be changed by the language
There was a problem hiding this comment.
But if, in my test, I compute the value, put it in an object, and then serialize that, if the serialization adds a space it would not work.
But this is conjecture. I don't know of any particular JSON serializer that would do it, and generally they like to compress as much as possible.
|
Published in |
2 participants
After this PR is merged it will only be necessary to add and document the corresponding capability for xcui driver
FYI @vrunoa