-
Notifications
You must be signed in to change notification settings - Fork 23
Fix/issue#186 path like support #191
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- modifications and updated docstrings, no tests
- modified Operator.connect() and updated docstring, no tests. Removed prior direct changes to operators
…sts types. Several tests in test_pathsupport.py covering most (all?) points of entry for paths. Modified test_elements.py test_descriptor_with_int_value which was getting allkindofcomplexity for no apparent reason.
Parameters | ||
---------- | ||
server_folder_path : str | ||
server_folder_path : str or os.PathLike |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PProfizi could you please download the built doc out of the github actions and make sure that the API documentation of what you changed looks good? Because I'm afraid the "or" will not, since we always use ","
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cbellot000
Ok I'll change that. I started using "or" because a lot of times there was already "str, optional" and adding a coma seemed more confusing to me (like: "str, os.PathLike, optional").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cbellot000 I found an example of docstring similar to what I did here:
fields_container (ansys.grpc.dpf.collection_pb2.Collection or FieldsContainer, optional)
in
class ansys.dpf.core.custom_fields_container.ElShapeFieldsContainer(fields_container=None, server=None)
ansys/dpf/core/path_utilities.py
Outdated
server = None | ||
parts = [] | ||
for a in args: | ||
if isinstance(a, str) and len(a) > 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can make it simpler with:
if isinstance(a, (str, Path)) and len(str(a)) > 0:
parts.append(str(a))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing is for an empty Path, its str representation is "." which is not of length==0, thus my choice of doing a separate test. If I understood correctly, this is to check the path is not empty (so an empty string OR an empty Path then?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, didnt ping you when replying @cbellot000
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or we could do:
if isinstance(a, (str, Path)) and Path(a) != Path(""):
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jfthuong works for me! Modifying now.
#186
Add support for pathlib.Path and other os.PathLike objects as inputs for file paths.
to detect where tests fail due to a Path object being given instead of a string.