search

ReviewNB / treon

Easy to use test framework for Jupyter Notebooks
https://reviewnb.com
MIT License
305 stars 29 forks source link

Add functionality for ignoring notebooks #13

Closed natanlao closed 5 years ago

natanlao commented 5 years ago

Closes #1. Allows ignoring notebooks with a .treonignore file. It mimics how .gitignore files work, except a little simpler.

amit1rrr commented 5 years ago

Thanks for your contribution @natanlao

We can actually simplify this quite a bit. From requirements perspective, we don't need to support multiple .treonignore files, just one file at the base of test PATH specified to treon should suffice. What we actually need though is to be able to specify individual file paths to ignore e.g. run all tests except ignore this scratch notebook.

With these requirements in mind we can simply implement the following,

  1. read all entries in a single .treonignore file (if present at base of test PATH)
  2. From our list of notebooks to test, remove the notebooks whose path overlaps with any entry in our ignore list

Edit: Updated my own suggestions.

natanlao commented 5 years ago

From requirements perspective, we don't need to support multiple .treonignore files, just one file at the base of test PATH specified to treon should suffice.

I'm not sure that only looking for a single .treonignore file would make this code materially simpler. I think that both approaches are similar in terms of both complexity and LOC. That said, I'm happy to change it if you still think it's a good idea.

What we actually need though is to be able to specify individual file paths to ignore e.g. run all tests except ignore this scratch notebook.

That works! https://github.com/ReviewNB/treon/blob/833ccded93ad561d0565c9728a682e780bb46f4b/tests/test_ignore.py#L30

amit1rrr commented 5 years ago

@natanlao I'm sorry I haven't been able to find free time to review this properly. I want to clone your changes and try it out locally for a meaningful review. I will get to it early next week. Sorry again for the delay.

natanlao commented 5 years ago

@amit1rrr All good! Thanks for your time.

natanlao commented 5 years ago

Thanks for the review @amit1rrr! I've addressed your comments and added tests cases where appropriate.

amit1rrr commented 5 years ago

I tried it again. Somehow the ignore list is still not working for me. Posted example in a separate comment.

I honestly think that this approach is getting too complicated in our quest to be identical as .gitignore (which we don't need to be). How about we simply read a single .treonignore file at base of the path. And exclude that rule list from notebooks_to_test by simply comparing whether the notebook path starts with any rule (if yes, ignore).

Some benefits over current approach,

natanlao commented 5 years ago

Okay! Sounds good to me.

amit1rrr commented 5 years ago

Discarding this PR now that we have --exclude command line option.

natanlao commented 4 years ago

Sorry, this got stuck on my backlog! Thanks for following up.

amit1rrr commented 4 years ago

No worries @natanlao