[GitHub] [tvm] comaniac commented on a change in pull request #7651: [TVMC] Allow options on --target to contain dots.

[GitBox <gi...@apache.org>]

comaniac commented on a change in pull request #7651:
URL: https://github.com/apache/tvm/pull/7651#discussion_r593346103



##########
File path: python/tvm/driver/tvmc/common.py
##########
@@ -133,7 +133,7 @@ def tokenize_target(target):
 
     target_pattern = (
         r"(\-{0,2}[\w\-]+\=?"
-        r"(?:[\w\+\-]+(?:,[\w\+\-])*|[\'][\w\+\-,\s]+[\']|[\"][\w\+\-,\s]+[\"])*|,)"
+        r"(?:[\w\+\-\.]+(?:,[\w\+\-\.])*|[\'][\w\+\-,\s\.]+[\']|[\"][\w\+\-,\s\.]+[\"])*|,)"

Review comment:
       Agree. This regex is super complex so it would be better to comment what's doing.

##########
File path: tests/python/driver/tvmc/test_tvmc_common.py
##########
@@ -273,3 +273,24 @@ def test_parse_multiple_target_with_opts():
     assert "myopt" in targets[0]["opts"]
     assert "value" == targets[0]["opts"]["myopt"]
     assert "llvm" == targets[1]["name"]
+
+
+def test_parse_multiple_separators_on_target():

Review comment:
       IMHO, it's fine to put them to one test. You could put the current test name to be the error message when assertation failure of each case.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org