-
Notifications
You must be signed in to change notification settings - Fork 236
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
Environment table #286
base: master
Are you sure you want to change the base?
Environment table #286
Conversation
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.
Great start!
Please see comments. :)
@@ -336,6 +336,38 @@ def _append_video(self, extra, extra_index, test_index): | |||
) | |||
self.additional_html.append(html.div(html_div, class_="video")) | |||
|
|||
class EnvironmentTable: | |||
def __init__(self, config): |
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 would split this big init up into smaller functions.
Think builder pattern.
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.
Not really sure to understand what to do here, I agree that everything in the init is not good but the function I can think of (generate_header, generate_row, ...) are very small... Do you want something like that?
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.
Yeah, I'm having trouble articulating the vision I have in my head. Give me the weekend to see if I can throw something together that you can refine. :)
Regarding builder pattern. We can have a default configuration and then users can use hooks to add to that config. |
Hi @BeyondEvil I just pushed something, tell me if it is what you are thinking when you say "builder pattern" |
How about a rebase? |
a064b92
to
8e14277
Compare
@ssbarnea Rebase done but there are failing tests, I guess they are new, I'll have a look |
I'd like to see a revamp of this for v4 if @werdeil is up for it. |
Following remarks made on PR 281, here is a first version of the EnvironmentTable class which allow to use new hooks.
@BeyondEvil tell me if it is what you had in mind.