Transport-for-the-North / caf.toolkit

Toolkit of transport planning and appraisal functionalities.
https://caftoolkit.readthedocs.io/en/stable/
Other
0 stars 2 forks source link

Added comments to BaseConfig saving YAML #59

Closed wsp-mbuckley closed 1 year ago

wsp-mbuckley commented 1 year ago

Describe Changes

Added optional datetime and custom comments to the top of YAML config files. Added optional comments to the write example method.

Address #57

Task Checklist

wsp-mbuckley commented 1 year ago

@isaac-tfn this will add date / time comments when saving to YAML by default and provides another parameter for custom comments if required, let me know what you think

wsp-mbuckley commented 1 year ago

Everything looks good apart from my comments on the typing not being backwards compatible.

Thanks for having a look, I've sorted that now so it should be ready to merge

isaac-tfn commented 1 year ago

There's a test failing which hasn't been touched in this PR (test_array_utils.TestSparseSum). Can you spot a reason why @BenTaylor-TfN?

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage is 100.00% of modified lines.

Files Changed Coverage
src/caf/toolkit/config_base.py 100.00%

:loudspeaker: Thoughts on this report? Let us know!.

isaac-tfn commented 1 year ago

There's a test failing which hasn't been touched in this PR (test_array_utils.TestSparseSum). Can you spot a reason why @BenTaylor-TfN?

I've just spotted there is a comment in the test that this is a known bug

wsp-mbuckley commented 1 year ago

There's a test failing which hasn't been touched in this PR (test_array_utils.TestSparseSum). Can you spot a reason why @BenTaylor-TfN?

I've just spotted there is a comment in the test that this is a known bug

Is this a linux specific issue because the test is passing fine on my machine

BenTaylor-TfN commented 1 year ago

Just spotted that this has also just happened on main, with some code that was previously fine before the PR was merged. See here: https://github.com/Transport-for-the-North/caf.toolkit/actions/runs/5953829958/job/16148999342

Here, it happened in python 3.11 and windows, must be something random causing it. Will try narrow it down shortly. Once I've logged this error, I'll re-run the test and it will likely pass.

BenTaylor-TfN commented 1 year ago

@wsp-mbuckley , sorted now

wsp-mbuckley commented 1 year ago

@BenTaylor-TfN or @isaac-tfn I think both of you are happy with this, please could one of you approve it and I'll merge it into main 😀