-
Notifications
You must be signed in to change notification settings - Fork 1
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
Improve test action #42
Conversation
gepetuto/test.py
Outdated
"""Get the dictionary of ipynbs to test.""" | ||
ipynbs = {} | ||
for ipynb in Path().glob("*.ipynb"): | ||
if str(ipynb)[0].isdigit(): |
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 strategy won't work with numbers like 10 or 5.1.
I think we might want to go full text here. We can decide a better convention for the naming, maybe {prefix}-{description}.ipynb
, where prefix and description can't contain -
, so that we can just ipynb.split('-')[0]
where needed.
We can rename eg. 1{_ → -}example_notebook.ipynb
and appendix{_ → _1-}scipy_optimizers.ipynb
, and decide the folders will be called {prefix}
instead of tp{prefix}
.
1c14086
to
893ddff
Compare
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 looks mostly ok, but maybe we should add an appendix-something.ipynb
in the test folder
gepetuto/test.py
Outdated
if prefix.isdecimal(): | ||
ipynbs = add_value_in_ipynbs(ipynbs, ipynb, prefix) | ||
else: | ||
ipynbs = add_value_in_ipynbs(ipynbs, ipynb, prefix) |
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.
both branches are doing the same thing
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.
Thanks, this looks good to me, but it's getting hard to know if this will work with prefixes like 5.1 or appendix without more test cases.
Do you want to add some here, or should I merge now as is ?
you're right I'm gonna add few files and unit tests for this |
No description provided.