Group #2 #39
Group #2 #39
Conversation
alextsakpinis
requested review from
NumericalMax,
SeverinBeger and
BaochangZhang
| string_io.writelines(["!include -setup .", page_path_name, "\n"]) | ||
|
|
||
| setup_path_name = HtmlUtil.get_Path(wiki_page=wiki_page, page_name=TestType.SETUP) | ||
| string_io.writelines(["!include -setup .", page_path_name, "\n"]) |
There was a problem hiding this comment.
Please rework this copy + paste error: this line uses "page_path_name", while the line before declares "setup_path_name"
| class TestType: | ||
| SETUP: "SetUp" | ||
| TEARDOWN: "TearDown" |
There was a problem hiding this comment.
Strictly speaking, it is not wrong to present the different types of tests using a class. However, the use of a dictionary or an enumeration is more lightweight and appropriate in this context.
| class HtmlUtil: | ||
|
|
||
| @staticmethod | ||
| def get_Path(page_name: str, wiki_page: WikiPage): |
There was a problem hiding this comment.
get_Path seems to be private. Consider putting it beyond the publicly callable function and indicate the intended access-level by an underscore, i.e., _get_path.
| page_path_name: str = path_parser.render(page_path) | ||
| string_io.writelines(["!include -teardown .", page_path_name, "\n"]) | ||
| page_path_name: str = HtmlUtil.get_Path(wiki_page=wiki_page, page_name=SuiteResponder.SUITE_TEARDOWN_NAME) | ||
| string_io.writelines(["!include -teardown .", page_path_name, "\n"]) |
There was a problem hiding this comment.
As a suggestion: it might be reasonable to join the functionalities of finding a page name and adding it to the io string under one function "_add_pathname_to_io", as this combination is used 4 times.
| path: wiki_page.get_page_crawler().get_full_path(setup) | ||
| path_name: str = path_parser.render(page_path) | ||
| return path_name | ||
|
|
||
| @staticmethod | ||
| def testable_html(page_data: PageData, include_suite_setup: bool, page_crawler: PageCrawlerImpl, path_parser: PathParser) -> str: | ||
| wiki_page: WikiPage = page_data.get_wiki_page() | ||
| string_io: StringIO = StringIO() | ||
|
|
||
| if page_data.has_attribute("Test"): |
There was a problem hiding this comment.
You could calculate page_data.has_attribute("Test") on top of the function body, or in a separate function once, as the boolean information is needed a second time (cf. line 32).
| if inherit_Page is not None: | ||
| path: wiki_page.get_page_crawler().get_full_path(setup) | ||
| path_name: str = path_parser.render(page_path) | ||
| return path_name |
There was a problem hiding this comment.
Please consider not returning NULL in case the "inherit_Page" is None (this condition has not been taken care of). Alternatively, you need to account for this scenario when using the function.
| path: wiki_page.get_page_crawler().get_full_path(setup) | ||
| path_name: str = path_parser.render(page_path) |
There was a problem hiding this comment.
Consider using error catching. In both steps sth. different can go wrong.
|
|
||
| page_data.set_content(string_io.getvalue()) | ||
| return page_data.get_html() | ||
|
|
||
|
|
There was a problem hiding this comment.
Add only one additional line at the end of the file. It might help to use a linter.
| tear_down_path: WikiPagePath = wiki_page.get_page_crawler().get_full_path(teardown) | ||
| tear_down_path_name: str = path_parser.render(tear_down_path) | ||
| string_io.writelines(["!include -teardown .", tear_down_path_name, "\n"]) | ||
| tear_down_path_name: str = HtmlUtil.get_Path(wiki_page=wiki_page, page_name=TestType.TEARDOWN) |
There was a problem hiding this comment.
The arguments are switched. HtmlUtil.get_Path(page_name=TestType.TEARDOWN, wiki_page=wiki_page)
There was a problem hiding this comment.
The same applies to the second get_Path call (cf. line 36)
Refactored code using a function