GHA: Run tests on windows-latest, macos-latest, ubuntu-latest #91
Conversation
Fails on Windows
Codecov Report
@@ Coverage Diff @@
## develop #91 +/- ##
===========================================
+ Coverage 79.76% 79.79% +0.02%
===========================================
Files 26 26
Lines 2941 2950 +9
Branches 713 714 +1
===========================================
+ Hits 2346 2354 +8
Misses 421 421
- Partials 174 175 +1
Continue to review full report at Codecov.
|
This reverts commit fdfd221.
petab/problem.py
Outdated
| path_prefix = os.path.dirname(yaml_config) | ||
| get_path = lambda filename:\ | ||
| f"{path_prefix}{os.sep}{filename}" # noqa: E731 |
There was a problem hiding this comment.
| path_prefix = os.path.dirname(yaml_config) | |
| get_path = lambda filename:\ | |
| f"{path_prefix}{os.sep}{filename}" # noqa: E731 | |
| get_path = lambda filename: \ | |
| yaml_config.parent / filename # noqa: E731 |
petab/problem.py
Outdated
| if isinstance(yaml_config, (str, Path)): | ||
| if isinstance(yaml_config, Path): |
There was a problem hiding this comment.
| if isinstance(yaml_config, (str, Path)): | |
| if isinstance(yaml_config, Path): | |
| if isinstance(yaml_config, Path): |
Then change else on line 168 to elif isinstance(yaml_config, str) and reduce indentation level by one for the block.
petab/problem.py
Outdated
| # yaml_config may be path or URL | ||
| path_url = urlparse(yaml_config) | ||
| if not path_url.scheme or \ | ||
| (path_url.scheme != 'file' and not path_url.netloc): | ||
| # a regular file path string | ||
| path_prefix = os.path.dirname(yaml_config) | ||
| get_path = lambda filename: \ | ||
| f"{path_prefix}{os.sep}{filename}" # noqa: E731 |
There was a problem hiding this comment.
Can the isinstance(..., Path) check above be removed and be handled by this same code (by changing the if condition here)?
There was a problem hiding this comment.
Could convert it to string and then handle it here. In the Path case, this will become unnecessarily complex, but would be shorter. Okay.
petab/problem.py
Outdated
| # a URL | ||
| # extract parent path from | ||
| url_path = unquote(urlparse(yaml_config).path) | ||
| parent_path = str(PurePosixPath(url_path).parent) | ||
| path_prefix = urlunparse( | ||
| (path_url.scheme, path_url.netloc, parent_path, | ||
| path_url.params, path_url.query, path_url.fragment) | ||
| ) | ||
| # need "/" on windows, not "\" | ||
| get_path = lambda filename: \ | ||
| f"{path_prefix}/{filename}" # noqa: E731 |
There was a problem hiding this comment.
All three of these code blocks for Path, str and URL seem quite similar, perhaps they can be combined converting them a type that supports the / operator then just define get_path once?
There was a problem hiding this comment.
No, this is unfortunately not possible, because we need different path separators. Thanks to Windows. The previous version was more concise, but didn't work on Windows...
petab/problem.py
Outdated
| else: | ||
| path_prefix = "" | ||
| get_path = lambda filename: filename # noqa: E731 |
There was a problem hiding this comment.
Can move out of else to before if, as the default
petab/visualize/data_overview.py
Outdated
| def create_report( | ||
| problem: petab.Problem, | ||
| model_name: str, | ||
| outdir: Union[str, Path] = '' |
There was a problem hiding this comment.
| outdir: Union[str, Path] = '' | |
| output_path: Union[str, Path] = '', |
petab/visualize/data_overview.py
Outdated
| """Create an HTML overview data / model overview report | ||
|
|
||
| Arguments: | ||
| problem: PEtab problem | ||
| model_name: Name of the model, used for file name for report | ||
| outdir: Output directory | ||
| """ | ||
|
|
||
| template_dir = os.path.join(os.path.dirname(os.path.abspath(__file__)), |
There was a problem hiding this comment.
| template_dir = os.path.join(os.path.dirname(os.path.abspath(__file__)), | |
| output_path = Path(output_path) | |
| template_path = output_path.parent.absolute() / 'templates' |
But then may need to use str(template_path) later
petab/visualize/data_overview.py
Outdated
| @@ -39,9 +46,9 @@ def create_report(problem: petab.Problem, model_name: str) -> None: | |||
| output_text = template.render(problem=problem, model_name=model_name, | |||
| data_per_observable=data_per_observable, | |||
| num_conditions=num_conditions) | |||
| with open(model_name + '.html', 'w') as html_file: | |||
| with open(Path(outdir) / f'{model_name}.html', 'w') as html_file: | |||
There was a problem hiding this comment.
| with open(Path(outdir) / f'{model_name}.html', 'w') as html_file: | |
| with open(output_path / f'{model_name}.html', 'w') as html_file: |
petab/visualize/data_overview.py
Outdated
| html_file.write(output_text) | ||
| copyfile(os.path.join(template_dir, 'mystyle.css'), 'mystyle.css') | ||
| copyfile(Path(template_dir, 'mystyle.css'), Path(outdir, 'mystyle.css')) |
There was a problem hiding this comment.
| copyfile(Path(template_dir, 'mystyle.css'), Path(outdir, 'mystyle.css')) | |
| copyfile(template_path / 'mystyle.css', output_path / 'mystyle.css') |
petab/visualize/data_overview.py
Outdated
| def main(outdir: Union[Path, str] = None): | ||
| """Data overview generation with example data from the repository for | ||
| testing | ||
| """ |
There was a problem hiding this comment.
Why is this test case code here and not in a test script?
Closes #80