Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ENH adding
write_html
toTableReport
#1190base: main
Are you sure you want to change the base?
ENH adding
write_html
toTableReport
#1190Changes from 10 commits
e8dfaf6
58b8a65
048cec5
15c7b06
0debd0b
260d710
684a9c1
597bc0e
ef50a50
bbb50d9
f3c1e8b
760b7b3
0789ac5
911eb5a
036db59
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 234 in skrub/_reporting/_table_report.py
Codecov / codecov/patch
skrub/_reporting/_table_report.py#L234
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.
Could you add a comment explaining what
html
is expected (or not expected) to be at line 230?Additionally, light inline documentation/comments on the steps above would help readability :)
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 test is looking great! the last thing we need to take care of is to make sure we close the file if we opened it (otherwise we "leak" a resource: the opened file handle. that could be a problem for example to clean up the temp directory on windows as it refuses to remove files that have an open file handle). Usually we ensure that with a simple context manager like:
but here we have a tricky situation because in some cases we have a string or path (which require no closing) and sometimes we have a file object which does require closing.
The standard library module
contextlib
provides 2 ways to deal with that situation easily. The first isExitStack
: it creates a context and we can push as many context managers as we want to its stack; when it exits it unwinds the stack, calling each manager's__exit__
when it is popped. So we could use it like:This option using ExitStack is my favorite because the file is being managed by a context manager as soon as it is opened.
Another way is to use
nullcontext
in the cases where we do not open the file, so that later we can treat all options as if they were open files that implement the context manager protocol.nullcontext
returns an object that implements the context manager protocol but whose__enter__
just returns the object we gave it and__exit__
does nothing: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.
wow contextlib.ExitStack() is very nice
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.
Maybe you could add a comment on L139 to explain why it's a good idea to use contextlib 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.
here also we want to use a context manager to close the file
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.
otherwise the assert is never executed because of the exception raised on the line above
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 actually change this to check if the file doesn't contain
html
to make sure we don't modify it.