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.

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

Allowing user to specify a target version #58

Merged
merged 10 commits into from
Dec 13, 2021
Merged

Conversation

nmerket
Copy link
Member

@nmerket nmerket commented Dec 7, 2021

Fixes #56

@nmerket nmerket requested a review from shorowit December 7, 2021 22:46
@nmerket nmerket self-assigned this Dec 7, 2021
@github-actions
Copy link

github-actions bot commented Dec 7, 2021

File Coverage Missing
All files 98%
__init__.py 100%
converter.py 98% 307, 368, 475, 629-630, 866-867
exceptions.py 100%

Minimum allowed coverage is 95%

Generated by 🐒 cobertura-action against d49a19f

Copy link
Member Author

@nmerket nmerket left a comment

Choose a reason for hiding this comment

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

Sorry about all the diffs. I used an automatic formatter (black) that changed a lot of things. I also:

  • added type hints to the functions
  • deprecated the convert_hpxml_to_3 function. It'll throw a warning if someone uses it. It's not used in the default cli anymore.

Comment on lines 77 to 99
def convert_hpxml_to_version(
hpxml_version: int, hpxml_file: File, hpxml_out_file: File
) -> None:
schema_version = detect_hpxml_version(hpxml_file)
major_version = schema_version[0]
if major_version == 1:
hpxml2_file = tempfile.NamedTemporaryFile()
convert_hpxml1_to_2(hpxml_file, hpxml2_file)
convert_hpxml2_to_3(hpxml2_file, hpxml3_file)
elif major_version == 2:
convert_hpxml2_to_3(hpxml_file, hpxml3_file)
if hpxml_version <= major_version:
raise exc.HpxmlTranslationError(
f"HPXML version requested is {hpxml_version} but input file version is {major_version}"
)
version_translator_funcs = {1: convert_hpxml1_to_2, 2: convert_hpxml2_to_3}
if hpxml_version - 1 not in version_translator_funcs.keys():
raise exc.HpxmlTranslationError(f"HPXML version {hpxml_version} not available")
current_file = hpxml_file
with tempfile.TemporaryDirectory() as tmpdir:
for current_version in range(major_version, hpxml_version):
next_version = current_version + 1
next_file = (
hpxml_out_file
if current_version + 1 == hpxml_version
else pathlib.Path(tmpdir, f"{next_version}.xml")
)
version_translator_funcs[current_version](current_file, next_file)
current_file = next_file
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the most pertinent code.

Comment on lines 21 to 22
type=int,
default=3,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we ask for the full version number? A software tool might very specifically restrict the version of HPXML they want. For example, it might require v2.0, whereas if the user specified 2 here they would get v2.3 automatically. We might as well be as useful as possible, right?

It wouldn't fundamentally change any of the logic in the translator, just the final version number that ends up in the HPXML file.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I 100% agree with the argument that it wouldn't change the logic of the translator. We'd also need to have all the minor version schemas available to validate against as well to make sure we didn't add anything extra.

hpxml_version_translator/__init__.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
test/test_converter_cli.py Outdated Show resolved Hide resolved
@nmerket nmerket merged commit 66dd58b into main Dec 13, 2021
@nmerket nmerket deleted the target_version_flag branch December 13, 2021 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add argument for target version?
2 participants