-
Notifications
You must be signed in to change notification settings - Fork 153
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
Migrate docstrings to doctests #901
Conversation
os.remove(dir_path + "1.txt") | ||
os.remove(dir_path + "2.txt") | ||
os.remove(dir_path + "3.txt") |
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.
Should them be os.remove(os.join(dir_path, "1.txt"))
?
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.
filepath_fn constructs a filename and not a directory, so concatenation is correct here; I did not change the example itself and only added matching test cleanup
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.
Really appreciate this effort. Left a few comments above
6b4b965
to
daf54f5
Compare
|
||
import os | ||
|
||
os.remove(file_prefix + "1.txt") |
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.
filepath_fn constructs a filename and not a directory, so concatenation is correct here; I did not change the example itself and only added matching test cleanup
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.
Makes sense to me. nit: Can we do a for loop here?
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.
Thank you. Left a few minor comments
|
||
import os | ||
|
||
os.remove(file_prefix + "1.txt") |
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.
Makes sense to me. nit: Can we do a for loop here?
|
||
from torchdata.datapipes.iter import IterableWrapper, S3FileLister | ||
|
||
S3FileLister.__iter__ = lambda self: iter([]) |
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.
This is too hacky. How about construct an empty list as s3_prefixes
and add a comment about the pattern of each 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.
imho if we use an empty list of files, the example is not very descriptive. IterableWrapper(['s3://bucket-name/folder/', ...])
is necessary for the user to understand the example.
We could mock here, but we would basically do the same thing of emptying the list in order to make it work. But mocking would alter the example. So I find it difficult to achieve a good solution. Please note that the test setup is not shown in the example and that we don't really want to alter the example here.
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.
I'll try patching. As I cannot use a with block, I will use patch.start() in testsetup and patch.stop() in testcleanup
from torchdata.datapipes.iter import S3FileLoader | ||
|
||
S3FileLoader.__iter__ = lambda self: iter([]) |
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.
Ditto
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.
changed to patching in testsetup and cleaning up the patch in testcleanup
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.
I think it will be great to add a section in the contribution guide (can be done in a separate PR) about doctests
, briefly describing that it exists and is part of the docstring.
Perhaps somewhere in this section?
|
daf54f5
to
7ef8a04
Compare
7ef8a04
to
1cc566a
Compare
@NivekT has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
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.
LGTM!
Changes
Migrate docstrings to doctest
Use PEP8 style in code examples, e.g. add newlines between defs: