Add functionality to easily load output files from previous executions into a running app#58
Conversation
heidi-holm-4ss
requested review from
bjorn-einar-bjartnes-4ss and
grzegorz-juras-4ss
grzegorz-juras-4ss
left a comment
grzegorz-juras-4ss
left a comment
There was a problem hiding this comment.
I left a few comments, there are things that are worth discussing.
| def _get_all_previous_file_names(app_id, session): | ||
| """query all available results file from the EngineRoom application. Returns list of dicts""" | ||
| response = session.get( | ||
| f"https://api.4insight.io/v1.0/Applications/{app_id}/results" |
There was a problem hiding this comment.
Aren't there any config file in the package, that contain API url? I think it's a bit risky to hardcode api root url (https://api.4insight.io/) inside function definition, because it may be changed to different URL some day.
There was a problem hiding this comment.
I thing putting it in constants is fine for now, like it was, but the constants is not used here.
| response.raise_for_status() | ||
| results = response.json() | ||
| if not results: | ||
| raise ValueError(f"No results found for application ID {app_id}.") |
There was a problem hiding this comment.
Wouldn't it be more user friendly to return empty list when there's no results, instead of throwing exception? For first execution of app, it'll surely fail with exception, because there are no results so far.
| ) | ||
|
|
||
| REMOTE_FILE_PATH = Path(__file__).parent / "testdata/a_test_file.json" | ||
| API_BASE_URL = "https://api.4insight.io" |
There was a problem hiding this comment.
Could use the constants here now too.
There was a problem hiding this comment.
i struggled with setting the path correctly
grzegorz-juras-4ss
left a comment
grzegorz-juras-4ss
left a comment
There was a problem hiding this comment.
Looks fine now!
3 participants
Add functionality for easily loading output files from previous executions into a running app. You can either download all files, or a specific file