Remove unnecessary try except #104
Conversation
| class MaskOutputFile: | ||
| @staticmethod | ||
| def create_mask_point(coords: geojson.coords, properties: dict): | ||
| def create_mask_point(coords: geojson.coords, properties: Optional[dict] = None) -> geojson.Feature: | ||
| """Creates a Point based on the properties and coordinates given. | ||
|
|
||
| Arguments: | ||
| coords {geojson.coords} -- | ||
| Coordinates tuple (x,y) for the mask point. | ||
| properties {dict} -- Dictionary of properties |
There was a problem hiding this comment.
| properties {dict} -- Dictionary of properties | |
| properties {dict} -- Dictionary of properties | |
| properties {dict, optional} -- Dictionary of properties. Defaults to None. |
There was a problem hiding this comment.
The contributing page of the documentation suggets to use google style docstrings when writing new code. The docstrings throughout the code are not consistent at the moment. I'm working on a PR that will address this issue.
| ) | ||
|
|
||
| @staticmethod | ||
| def read_mask_output_file(file_path: str): | ||
| def read_mask_output_file(file_path: Union[str, Path]) -> dict: | ||
| """Imports a GeoJson from a given json file path. | ||
|
|
||
| Arguments: | ||
| file_path {str} -- Location of the json file |
There was a problem hiding this comment.
| file_path {str} -- Location of the json file | |
| file_path {str} -- Location of the json file | |
| file_path {Union[str, Path]} -- Location of the json file |
|
|
||
| with file_path.open("r") as geojson_file: | ||
| geojson_data = geojson.load(geojson_file) | ||
| if not isinstance(geojson_data, geojson.FeatureCollection): |
There was a problem hiding this comment.
Is this check actually needed? Can geojson_data be something else than a geojson.FeatureCollection?
There was a problem hiding this comment.
Yes, geojson will load a json as a dict. If the file is a geojson it will return a geojson.FeatureCollection
| return geojson_data | ||
|
|
||
| @staticmethod | ||
| def write_mask_output_file(file_path: str, mask_points: list): | ||
| def write_mask_output_file(file_path: Union[Path, str], mask_points: list) -> None: | ||
| """Writes a .geojson file with a Feature collection containing | ||
| the mask_points list given as input. | ||
|
|
||
| Arguments: | ||
| file_path {str} -- file_path where to store the geojson. |
There was a problem hiding this comment.
| file_path {str} -- file_path where to store the geojson. | |
| file_path {str} -- file_path where to store the geojson. | |
| file_path {Union[Path, str]} -- file_path where to store the geojson. |
| feature_collection = geojson.FeatureCollection(mask_points) | ||
| with open(file_path, "w") as f: | ||
| geojson.dump(feature_collection, f, indent=4) | ||
| file_path = Path(file_path) |
There was a problem hiding this comment.
I think you should first check if the given file_path is not None. If it is, Path(file_path) may (or may not, depending on the python version) raise an error. So better to do something like:
if not file_path:
raise ValueError("file_path is required.")
if not mask_points:
raise ValueError("mask_points cannot be empty.")
file_path = Path(file_path)
There was a problem hiding this comment.
Good suggestion, I added the check.
|
@tim-vd-aardweg Thanks again for the review! |
Linked issue: #102
I started out by removing unnecessary try except statements in the test. These statements would except any error and then call pytest.fail with the message that the error is unexpected with the respective error message. Seems quite redundant to me.
I had to fix some tests after removing the try except statement and I ended up re-writing test_maskoutputfile.py and changing quite some code MaskOutputFile.py