Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix Bug in #73 #76

Merged
merged 6 commits into from
Jan 31, 2020
Merged

Fix Bug in #73 #76

merged 6 commits into from
Jan 31, 2020

Conversation

hamelsmu
Copy link
Contributor

@sgugger I introduced a bug in #73 🤦‍♂ . I am really sorry about this. To try to mitigate this from happening I added tests for notebook2html. I am not 100% sure this is the way you want to see tests, but I tried to follow the conventions in the notebook carefully.

Happy to amend this PR with any suggestions. 🙇 Thank you

@hamelsmu hamelsmu changed the title Backticks Fix Bug in #73 Jan 30, 2020
@hamelsmu hamelsmu requested a review from sgugger January 30, 2020 19:48
else: files = list(p.parent.glob(p.name))
else:
p = Path(fname)
files = list(p.parent.glob(p.name))
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 was the bug. I forgot to include p = Path(fname). I realized that I didn't catch this error because I didn't have great tests. So I added some tests to this notebook.

@hamelsmu
Copy link
Contributor Author

hamelsmu commented Jan 30, 2020

I also turned on CI in my local repo and can verify the latest commit on this PR's branch it is passing

image

hamelsmu referenced this pull request Jan 30, 2020
@sgugger
Copy link

sgugger commented Jan 31, 2020

Thanks! I guess I should also have read more carefully ;)
The test seem fine, I may just refactor them using fastcore one of these days.

@sgugger sgugger merged commit 2e14737 into AnswerDotAI:master Jan 31, 2020
@sgugger
Copy link

sgugger commented Jan 31, 2020

Follow-up question. Do you know if there is a way to automatically trigger the CI at each commit or at the least with an action form me before merging (like adding a special label)? That would help ensure these kinds of breaks don't happen.

@hamelsmu
Copy link
Contributor Author

I’m looking into this now, will comment back on this thread when I find the best solution

@hamelsmu
Copy link
Contributor Author

Opened #77 to address this

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

Successfully merging this pull request may close these issues.

2 participants