PowerGridModel / power-grid-model-io

Conversion tool for various grid data formats to power-grid-model
Mozilla Public License 2.0
12 stars 8 forks source link

Temporary workaround converting Vision 97 file #227

Closed Jerry-Jinfeng-Guo closed 6 months ago

Jerry-Jinfeng-Guo commented 6 months ago

Standalone script as a temporaty workaround converting Vision 97 file to old id scheme. This relates to https://github.com/PowerGridModel/power-grid-model-io/issues/225

Jerry-Jinfeng-Guo commented 6 months ago

This is a standalone script. The following two are to be decided:

Jerry-Jinfeng-Guo commented 6 months ago

most of it looks clear to me, but i don't particularly like the idea of having a temporary workaround script. what's the rationale?

As mentioned in https://github.com/PowerGridModel/power-grid-model-io/issues/225#issue-2133779122, there are some things yet to be decided within the team regarding the proper solutions and this temporary workaround serves as a compromise.

mgovers commented 6 months ago

This is a standalone script. The following two are to be decided:

* Not entirely sure where it should go. Currently it lives inside the utils directory.

Please make a separate scripts/ directory at the root.

* Not sure if a test case is needed. Currently code coverage will be 0.

I do think that's a good idea. A small one should suffice. It should also fix the overall coverage criterion on which CI currently fails

My expectation is that a semi-sustainable solution will involve including this code into the existing VSF converter. Actually, what is the reason you're not able to do this now?

Jerry-Jinfeng-Guo commented 6 months ago

This is a standalone script. The following two are to be decided:

* Not entirely sure where it should go. Currently it lives inside the utils directory.

Please make a separate scripts/ directory at the root.

* Not sure if a test case is needed. Currently code coverage will be 0.

I do think that's a good idea. A small one should suffice. It should also fix the overall coverage criterion on which CI currently fails

My expectation is that a semi-sustainable solution will involve including this code into the existing VSF converter. Actually, what is the reason you're not able to do this now?

Like what is the reason for anything ;) The reason is that the proper solution (which I assume won't be simply converting the document) would take more time when this could be released to people to use already. I'll wait for some input on that one next Tuesday before I potentially spend more than necessary on it.

mgovers commented 6 months ago

Like what is the reason for anything ;) The reason is that the proper solution (which I assume won't be simply converting the document) would take more time when this could be released to people to use already. I'll wait for some input on that one next Tuesday before I potentially spend more than necessary on it.

that's why a small test case should suffice. it's useful to have in the first place but you don't have to spend too much time on it

Jerry-Jinfeng-Guo commented 6 months ago

Like what is the reason for anything ;) The reason is that the proper solution (which I assume won't be simply converting the document) would take more time when this could be released to people to use already. I'll wait for some input on that one next Tuesday before I potentially spend more than necessary on it.

that's why a small test case should suffice. it's useful to have in the first place but you don't have to spend too much time on it

Yup, agreed. It's on the way. But I have to leave for the appointment now. There was a tiny bug that was almost gone. New commit will be pushed and ready for review after lunch.

sonarcloud[bot] commented 6 months ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
90.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud