Skip to content
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.

Sign up for GitHub

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

Jump to bottom

#104: Skip serialization of empty INI properties when configured #336

Merged
merged 12 commits into from
Oct 13, 2022
Merged

#104: Skip serialization of empty INI properties when configured #336

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
f0101c9
#104: Fix documentation; the default is False.
priscavdsluis Sep 6, 2022
24c5ca6
#104: Add skip_empty_properties property to SerializerConfig and add …
priscavdsluis Sep 6, 2022
7fc7c85
#104: Skip empty properties during serialization when skip_empty_prop…
priscavdsluis Sep 6, 2022
e46dc40
autoformat: isort & black
priscavdsluis Sep 6, 2022
260df7e
#104: Set SerializerConfig skip_empty_properties property to False fo…
priscavdsluis Sep 8, 2022
4fcc59c
autoformat: isort & black
priscavdsluis Sep 8, 2022
48992e9
#104: Fix documentation
priscavdsluis Sep 8, 2022
86571ae
#104: Add property with white space value to test case.
priscavdsluis Sep 27, 2022
c0bc9b2
Merge branch 'main' into fix/104-skip-serialization-empty-properties
priscavdsluis Sep 28, 2022
c06fb3f
Merge branch 'main' into fix/104-skip-serialization-empty-properties
priscavdsluis Oct 10, 2022
8e3cb2c
#104: Add documentation to _serialize
priscavdsluis Oct 13, 2022
8b8656e
Merge branch 'main' into fix/104-skip-serialization-empty-properties
priscavdsluis Oct 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions hydrolib/core/basemodel.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -829,6 +829,7 @@ def _save(self) -> None:
self._serialize(self.dict())

def _serialize(self, data: dict) -> None:
"""Serializes the data to file. Should not be called directly, only through `_save`."""
path = self._resolved_filepath
if path is None:
# TODO: Do we need to add a warning / exception here
Expand Down
2 changes: 1 addition & 1 deletion hydrolib/core/io/ini/parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ class ParserConfig(BaseModel):
Attributes:
allow_only_keywords (bool):
Whether to allow properties with only keys (no '=' or value).
Defaults to True.
Defaults to False.
parse_datablocks (bool):
Whether to allow parsing of datablocks at the bottom of sections.
Defaults to False.
Expand Down
7 changes: 7 additions & 0 deletions hydrolib/core/io/ini/serializer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
Property,
Section,
)
from hydrolib.core.utils import str_is_empty_or_none


class SerializerConfig(BaseModel):
Expand All @@ -34,13 +35,16 @@ class SerializerConfig(BaseModel):
additional offset to ensure . is lined out. Defaults to 4.
comment_delimiter (str):
The character used to delimit comments. Defaults to '#'.
skip_empty_properties (bool):
Whether or not to skip properties with a value that is empty or None. Defaults to True.
"""

section_indent: int = 0
property_indent: int = 4
datablock_indent: int = 8
datablock_spacing: int = 4
comment_delimiter: str = "#"
skip_empty_properties: bool = True

@property
def total_property_indent(self) -> int:
Expand Down Expand Up @@ -192,6 +196,9 @@ def _serialize_content_element(self, elem: ContentElement) -> Lines:
return _serialize_comment_block(elem, delimiter, indent)

def _serialize_property(self, property: Property) -> Lines:
if self.config.skip_empty_properties and str_is_empty_or_none(property.value):
return

indent = " " * (self._config.total_property_indent)
key_ws = _get_offset_whitespace(property.key, self.max_length.key)
key = f"{property.key}{key_ws} = "
Expand Down
5 changes: 5 additions & 0 deletions hydrolib/core/io/mdu/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from hydrolib.core.io.ext.models import ExtModel
from hydrolib.core.io.friction.models import FrictionModel
from hydrolib.core.io.ini.models import INIBasedModel, INIGeneral, INIModel
from hydrolib.core.io.ini.serializer import SerializerConfig, write_ini
from hydrolib.core.io.ini.util import get_split_string_on_delimiter_validator
from hydrolib.core.io.inifield.models import IniFieldModel
from hydrolib.core.io.net.models import NetworkModel
Expand Down Expand Up @@ -820,3 +821,7 @@ def _get_relative_mode_from_data(cls, data: Dict[str, Any]) -> ResolveRelativeMo
return ResolveRelativeMode.ToAnchor
else:
return ResolveRelativeMode.ToParent

def _serialize(self, _: dict) -> None:
config = SerializerConfig(skip_empty_properties=False)
write_ini(self._resolved_filepath, self._to_document(), config=config)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Visual Studio gives the following error for self._resolved_filepath:

Argument of type "Path | None" cannot be assigned to parameter "path" of type "Path" in function "write_ini" Type "Path | None" cannot be assigned to type "Path" Type "None" cannot be assigned to type "Path"

I think it is because _resolved_filepath is Optional and write_ini() expects a non-optional path.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

before this method is called, the _resolved_filepath property is checked for None, so it cannot be None here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that so? I see such a check only in ParsableFileModel._serialize(), but that will not be called if a subclass overrides that method, for example in INIModel._serialize().

I think the place that now makes sure everything runs well is this:

class FileModel(BaseModel, ABC):
# ...
    def _save_instance(self) -> None:
        if self.filepath is None:

Should we not add documentation that _serialize() should always only be called via save() and never directly?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@priscavdsluis wrote that the latter is implicitly so, because _serialize() is a private function. That is true for end-user APIdocs. But amongst core-developers, it is good to know that other private functions should probably also not call _serialize directly. Maybe some one-liner docs in the function body?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some docs and will close this PR.

6 changes: 1 addition & 5 deletions tests/data/reference/fm/serialize_initialFields.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# written by HYDROLIB-core 0.1.5
# written by HYDROLIB-core 0.3.0

[General]
fileVersion = 2.00
Expand All @@ -16,21 +16,18 @@
averagingPercentile = 0 # Percentile value for which data values to include in averaging. 0.0 means off.
extrapolationMethod = 0 # Option for (spatial) extrapolation.
locationType = 2d
value = # Only for dataFileType=polygon. The constant value to be set inside for all model points inside the polygon.

[Initial]
quantity = bedlevel
dataFile = inibedlevel.ini
dataFileType = 1dField
interpolationMethod = # Type of (spatial) interpolation.
operand = O # How this data is combined with previous data for the same quantity (if any).
averagingType = mean # Type of averaging, if interpolationMethod=averaging .
averagingRelSize = 1.01 # Relative search cell size for averaging.
averagingNumMin = 1 # Minimum number of points in averaging. Must be ≥ 1.
averagingPercentile = 0 # Percentile value for which data values to include in averaging. 0.0 means off.
extrapolationMethod = 0 # Option for (spatial) extrapolation.
locationType = all # Target location of interpolation.
value = # Only for dataFileType=polygon. The constant value to be set inside for all model points inside the polygon.

[Parameter]
quantity = frictioncoefficient
Expand All @@ -44,7 +41,6 @@
averagingPercentile = 0 # Percentile value for which data values to include in averaging. 0.0 means off.
extrapolationMethod = 0 # Option for (spatial) extrapolation.
locationType = all # Target location of interpolation.
value = # Only for dataFileType=polygon. The constant value to be set inside for all model points inside the polygon.

[Parameter]
quantity = frictioncoefficient
Expand Down
10 changes: 1 addition & 9 deletions tests/data/reference/fm/serialize_roughness.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
# written by HYDROLIB-core 0.1.6*
# written by HYDROLIB-core 0.3.0

[General]
fileVersion = 3.01
fileType = roughness
frictionValuesFile = # Name of <*.bc> file containing the timeseries with friction values. Only needed for functionType = timeSeries.

[Global]
frictionId = Main
Expand All @@ -14,9 +13,6 @@
branchId = Channel1
frictionType = Manning
functionType = constant
timeSeriesId = # Refers to a data block in the <*.bc> frictionValuesFile. Only if functionType = timeSeries.
numLevels = # Number of levels in table. Only if functionType is not constant.
levels = # Space separated list of discharge [m3/s] or water level [m AD] values. Only if functionType is absDischarge or waterLevel.
numLocations = 2 # at two locations
chainage = 0.0 100.0
frictionValues = 0.2 0.3
Expand All @@ -25,10 +21,6 @@
branchId = Channel4
frictionType = Chezy
functionType = constant
timeSeriesId = # Refers to a data block in the <*.bc> frictionValuesFile. Only if functionType = timeSeries.
numLevels = # Number of levels in table. Only if functionType is not constant.
levels = # Space separated list of discharge [m3/s] or water level [m AD] values. Only if functionType is absDischarge or waterLevel.
numLocations = 0 # Number of locations on branch. The default 0 implies branch uniform values.
chainage = # Space separated list of locations on the branch [m]. Locations sorted by increasing chainage. The keyword must be specified if numLocations>0.
frictionValues = 40.0

65 changes: 61 additions & 4 deletions tests/io/test_ini.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -803,6 +803,15 @@ def test_total_datablock_indent_expected_result(self):
config = SerializerConfig(section_indent=3, datablock_indent=10)
assert config.total_datablock_indent == 13

def test_default_serializer_config(self):
config = SerializerConfig()
assert config.section_indent == 0
assert config.property_indent == 4
assert config.datablock_indent == 8
assert config.datablock_spacing == 4
assert config.comment_delimiter == "#"
assert config.skip_empty_properties == True


class TestLengths:
@pytest.mark.parametrize(
Expand Down Expand Up @@ -1260,13 +1269,17 @@ def test_serialize_section_header(
(
Property(key="key", value=None, comment=None),
MaxLengths(key=3, value=0),
SerializerConfig(section_indent=0, property_indent=0),
SerializerConfig(
section_indent=0, property_indent=0, skip_empty_properties=False
),
"key =",
),
(
Property(key="key", value=None, comment="comment"),
MaxLengths(key=3, value=5),
SerializerConfig(section_indent=0, property_indent=0),
SerializerConfig(
section_indent=0, property_indent=0, skip_empty_properties=False
),
"key = # comment",
),
(
Expand Down Expand Up @@ -1583,6 +1596,50 @@ def test_serialize_datablock(
" 1.0 16.0 81.0 256.0",
],
),
(
Section(
header="header",
content=[
Property(key="key1", value="value", comment="comment1"),
Property(key="key2", value="", comment="comment2"),
Property(key="key3", value=None, comment="comment3"),
],
datablock=[],
),
SerializerConfig(
section_indent=0,
property_indent=4,
datablock_indent=6,
datablock_spacing=4,
skip_empty_properties=False,
),
[
"[header]",
" key1 = value # comment1",
" key2 = # comment2",
" key3 = # comment3",
],
),
(
Section(
header="header",
content=[
Property(key="key1", value="value", comment="comment1"),
Property(key="key2", value="", comment="comment2"),
Property(key="key3", value=None, comment="comment3"),
priscavdsluis marked this conversation as resolved.
Show resolved Hide resolved
Property(key="key4", value=" ", comment="comment4"),
],
datablock=[],
),
SerializerConfig(
section_indent=0,
property_indent=4,
datablock_indent=6,
datablock_spacing=4,
skip_empty_properties=True,
),
["[header]", " key1 = value # comment1"],
),
],
)
def test_serialize_section(
Expand Down Expand Up @@ -1626,7 +1683,7 @@ def test_deserialize_serialize_should_give_the_same_result(self):

document = parser.finalize()

serializer = Serializer(config=SerializerConfig())
serializer = Serializer(config=SerializerConfig(skip_empty_properties=False))
result = "\n".join(serializer.serialize(document))

assert result == input_str
Expand Down Expand Up @@ -1720,7 +1777,7 @@ def test_serialize_deserialize_should_give_the_same_result():
)

path = test_output_dir / "tmp" / "test.pliz"
write_ini(path, document)
write_ini(path, document, config=SerializerConfig(skip_empty_properties=False))

parser = Parser(config=ParserConfig(parse_datablocks=True))
with path.open("r") as f:
Expand Down