Allow users to provide table names as pathlib.Path objects - #295
Conversation
Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
gmloose
left a comment
gmloose
left a comment
There was a problem hiding this comment.
I have some remarks. See above.
| _columnnames=[], _datatypes=[], | ||
| _oper=0, _delete=False): | ||
| """Open or create a table.""" | ||
| import pathlib |
There was a problem hiding this comment.
I would prefer to have imports at the top of the file, and not inside functions.
There was a problem hiding this comment.
Me too, but I was preferred to follow the precedent already established by many other functions in this file, where imports are done within each function.
| elif hasattr(name, "__iter__"): | ||
| return [_do_remove_prefix(nm) for nm in name] | ||
| return name |
There was a problem hiding this comment.
I'm not sure why this is needed; I don't see a direct connection with the support for pathlib.Path. Having said that, IMHO, the pythonic way to do this is:
try:
return [_do_remove_prefix(nm) for nm in name]
except TypeError:
return name
There was a problem hiding this comment.
This is needed so other utility functions like tableutil.tabledelete can also take a Path object (otherwise they'd fail while trying to iterate over a Path object, which isn't supported).
I did think about having something like what you suggest, but the try/except block could potentially catch TypeErrors that are raised within _do_remove_prefix, so that seemed slightly risky. Granted, that shouldn't happen at the moment, but it would be a hidden dependency between the two functions.
Alternatively, I could also add a more specific isinstance(name, Path) and go with that, since that's the more direct behaviour that is being sought.
| path = pathlib.Path("ttable.py_tmp.tab1") | ||
| t = table(path, maketabdesc([c1]), ack=False) | ||
| t.close() | ||
| tabledelete(path) |
There was a problem hiding this comment.
You may want to use with table(...) as t:, which is exception-safe, and doesn't require you to call t.close().
There was a problem hiding this comment.
Happy to do so; again I was following what other functions were doing already
|
@gmloose thanks for the review! Please let me know how you think I should proceed with each individual note, hopefully the rationale for each of them is clearer now. |
2 participants
In a few codebases we use
pathlib.Pathobjects for denoting paths in the filesystem. Howeverpython-casacorestringly requires astrfor itstablenamearguments throughout. This PR adds support for providing table names aspathlib.Pathobjects.