Added option to append one or more user-provided style sheets to report's styling. #150
Conversation
There was a problem hiding this comment.
I'm really sorry for not looking at this earlier. Thank you so much for submitting this patch! As you suggest in your description, I think the error handling could be improved, but otherwise I think this is awesome! Let me know if I can provide any additional help.
| @@ -55,6 +55,9 @@ def pytest_addoption(parser): | |||
| 'that the report may not render or function where CSP ' | |||
| 'restrictions are in place (see ' | |||
| 'https://developer.mozilla.org/docs/Web/Security/CSP)') | |||
| group.addoption('--append-css', action='append', dest='appendcss', | |||
There was a problem hiding this comment.
Can we name this simply '--css' and leave the default dest of 'css'?
| @@ -129,7 +135,7 @@ def __init__(self, outcome, report, logfile, config): | |||
| if len(cells) > 0: | |||
| self.row_table = html.tr(cells) | |||
| self.row_extra = html.tr(html.td(self.additional_html, | |||
| class_='extra', colspan=len(cells))) | |||
| class_='extra', colspan=len(cells))) | |||
There was a problem hiding this comment.
Nit: This change makes the line too long for PEP8.
| @@ -321,6 +327,21 @@ def _generate_report(self, session): | |||
| ansi_css.extend([str(r) for r in style.get_styles()]) | |||
| self.style_css += '\n'.join(ansi_css) | |||
|
|
|||
| # <DF> Add user-provided CSS | |||
| if self.css_append: | |||
There was a problem hiding this comment.
I believe if we're using append, then the default should be an empty list. If so, we shouldn't need this check and could just do for path in self.css:
| user_css.extend(css_file.readlines()) | ||
| user_css.append('/* End CSS from {} */'.format(filename)) | ||
| except Exception as e: | ||
| self.css_errors.append('Warning: Could not read CSS from {}: {}'.format(filename, e)) |
There was a problem hiding this comment.
Rather than gathering bare exceptions for later reporting, I think we should check that the paths are valid before w run any tests. This can be done in pytest_addoption or pytest_configure. Otherwise, a typo in any CSS path would not be known until the test session is complete, which could be frustrating if the tests take a long time to run.
| except Exception as e: | ||
| self.css_errors.append('Warning: Could not read CSS from {}: {}'.format(filename, e)) | ||
|
|
||
| self.style_css += '\n'.join(user_css) |
There was a problem hiding this comment.
I wonder if it makes sense to keep the CSS files separate instead of appending to a single file. We don't have a choice for the self-contained report, but it might help to keep the assets based report more manageble. We can always revisit this later though.
|
In addition to my review comments, could you look into getting this passing the flake8 checks? You can run them locally using |
|
Thanks again @i-am-david-fernandez for kicking this off. I've addressed my comments, and will now close this in favour of #156, which includes your valued contribution! |
Caveat: this is my first pull request, so opening apologies if I've not done things correctly. Constructive criticism on anything is welcome.
I'm not sure if I've approached the error reporting (failure to read user-supplied CSS file) appropriately, so feel free to alter that.
The approach I've taken allows multiple user-supplied CSS files to be added in the order specified. Perhaps this isn't necessary, but the difference is near trivial, and it provides quite a good increase in flexibility for essentially no cost. It may be useful if a team wants to apply something like business and team/project/client styling.