JavaScript parser improvement #1176
Conversation
aaltat
force-pushed
the
js_approval_tests
branch
from
918b48e to
eae867d
Compare
|
Fixes #323 |
|
Need to take a look on few things:
|
pekkaklarck
left a comment
pekkaklarck
left a comment
There was a problem hiding this comment.
Looks good. Some nitpickey line notes added. I'll review the code related to finding JS code and arguments when I got time.
| return self.driver.execute_script(js) | ||
| js_code, js_args = self._get_javascript_to_execute(code) | ||
| self.info('Executing JavaScript:\n%s\nBy using argument(s):\n"%s"' | ||
| % (js_code, ', '.join(js_args))) |
There was a problem hiding this comment.
Logging could be enhanced a little:
- Better not to mention anything about arguments if there are none. Current
""looks odd. - Instead of
"argument(s):"it's possible to use"argument%s:" % robot.utils.plural_or_not(js_args)to get the trailingswhen needed. - Quotes around arguments like
"first, second"look a bit strange. I'd consider omitting them altogether or alternatively quoting each argument separately. In the latter caserobot.utils.seq2strorseq2str2might be worth a look. Just loggingjs_argsas-is would be fine otherwise as well, but theuprefixes with Unicode strings inside a list in Python 2 are annoying.
There was a problem hiding this comment.
I did not know that such utils did exist. I definitely will take a look.
| return code | ||
| js_code, js_args = self._get_javascript_to_execute(code) | ||
| self.info('Executing Asynchronous JavaScript:\n%s\nBy using argument(s):\n"%s"' | ||
| % (js_code, ', '.join(js_args))) |
There was a problem hiding this comment.
Same comment about logging as above. Logging should probably be done using a helper.
There was a problem hiding this comment.
Helper is good idea. Will definitely take a look.
| js_code.append(line) | ||
| if get_args: | ||
| js_args.append(line) | ||
| return js_code, js_args |
There was a problem hiding this comment.
This looks a bit complicated. Don't have time to look at more closely could it be improved. Will look again later.
There was a problem hiding this comment.
Yes, it's complicated. At least raising an error could be moved to a separate method. Need to think what else could be done.
2 participants
No description provided.