Skip to content
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

Add maybe copy tests #980

Merged
merged 3 commits into from Oct 28, 2018
Merged

Add maybe copy tests #980

merged 3 commits into from Oct 28, 2018

Conversation

radekosmulski
Copy link
Contributor

To get my feet wet with pytest, picked something relatively straight forward from fastai.core. Fixed a couple of typos along the way and made minor change to maybe_copy.

Will start a thread to discuss.

Copy link
Member

@jph00 jph00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice tests! :) Just minor suggestions...

assert (df['col3'].dtypes == 'int64')

def _write_file(name):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These can be put on one line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this initially but was getting errors. Googled for this and seems you can skip one line feed, but not two.

The clean_up function is going away thx to TemporaryDirectory - let me see what I can do about the _write_file one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks nicer :)

def _write_file(dn, name): f = open(f'{dn}/{name}', 'w'); f.write(str(name)); f.close()


def test_copies_if_older(self):
_write_file('first')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably best to use the tempfile std lib and a with block to ensure that the files are created somewhere appropriate, and automatically cleaned up when done.

@radekosmulski
Copy link
Contributor Author

Rebased on master and started to squash commits out of habit - hoping changing history did not mess anything up with the workflow on github.

@jph00 jph00 merged commit 70cb432 into fastai:master Oct 28, 2018
@jph00
Copy link
Member

jph00 commented Oct 28, 2018

Great job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants