-
Notifications
You must be signed in to change notification settings - Fork 2
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
Feat/337 ra2ce result output exporter #357
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.
Some small questions/comments
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 assume this class needs an __init__
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 am not sure why, there's nothing required to be consumed. At least until further requirements are found we do not need more than this for exporting the current results. Either way we can have a chat about it, not a big deal.
@@ -115,7 +71,7 @@ def run( | |||
) | |||
_results.append(_result_wrapper) | |||
|
|||
self._save_result(_result_wrapper) | |||
AnalysisResultWrapperExporter().export_result(_result_wrapper) |
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.
What is not taken into account is that the various analysis modules/packages export much more data than is reflected in the ResultWrapper
. I think this needs some rethinking.
def test_given_valid_result_export_gdf( | ||
self, valid_result_wrapper: AnalysisResultWrapper | ||
): | ||
# 1. Define test data. | ||
valid_result_wrapper.analysis.analysis.save_gpkg = True | ||
self._export_valid_result_to_expected_format(valid_result_wrapper, "gpkg") | ||
|
||
def test_given_valid_result_export_csv( | ||
self, valid_result_wrapper: AnalysisResultWrapper | ||
): | ||
# 1. Define test data. | ||
valid_result_wrapper.analysis.analysis.save_csv = True | ||
self._export_valid_result_to_expected_format(valid_result_wrapper, "csv") |
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.
Why split this test in 2 and not parametrize it on e.g. output_format, adding some minimalistic logic?
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.
Because then I need to create if-else
s or other ugly logic to set up save_csv
or sve_gpkg
. This way they are still self-contained and explanatory. Code duplication is avoided thanks to the _export_valid_result_to_expected_format
.
…porter Feat/337 ra2ce result output exporter
No description provided.