open-sdg / sdg-build

Python package to convert SDG-related data and metadata between formats
MIT License
6 stars 22 forks source link

Halt build and provide better error messages on YAML syntax problems #336

Closed brockfanning closed 1 year ago

brockfanning commented 1 year ago

Fixes https://github.com/open-sdg/open-sdg/issues/1766

Testing instructions:

This PR includes the following changes:

  1. YAML syntax problems in metadata files and indicator configuration files will receive a more helpful error message.
  2. YAML syntax problems in translation files will receive a more helpful error message, and will now prevent the build from continuing.

I bolded part of the second item above because it is kind of a "breaking change" in that it may prevent builds from completing where previously the completed. However it could be argued that this breaking change is OK, because previously it was failing silently to import the translation files, which probably is causing confusion. So now it will fail, but will remove confusion.

Here is an example of the syntax problem this PR should now catch:

foo: 'hello 'open' sdg'

To test this, you can try adding the line above in any/all of:

  1. YAML indicator metadata files
  2. YAML indicator configuration files
  3. YAML translation files
codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.94 :tada:

Comparison is base (c78e8bd) 73.76% compared to head (810d461) 74.70%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## 2.2.0-dev #336 +/- ## ============================================= + Coverage 73.76% 74.70% +0.94% ============================================= Files 74 74 Lines 4577 4606 +29 ============================================= + Hits 3376 3441 +65 + Misses 1201 1165 -36 ``` | [Impacted Files](https://codecov.io/gh/open-sdg/sdg-build/pull/336?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-sdg) | Coverage Δ | | |---|---|---| | [sdg/helpers/files.py](https://codecov.io/gh/open-sdg/sdg-build/pull/336?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-sdg#diff-c2RnL2hlbHBlcnMvZmlsZXMucHk=) | `100.00% <100.00%> (ø)` | | | [sdg/inputs/InputYamlMdMeta.py](https://codecov.io/gh/open-sdg/sdg-build/pull/336?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-sdg#diff-c2RnL2lucHV0cy9JbnB1dFlhbWxNZE1ldGEucHk=) | `100.00% <100.00%> (ø)` | | | [sdg/inputs/InputYamlMeta.py](https://codecov.io/gh/open-sdg/sdg-build/pull/336?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-sdg#diff-c2RnL2lucHV0cy9JbnB1dFlhbWxNZXRhLnB5) | `100.00% <100.00%> (ø)` | | | [sdg/translations/TranslationInputYaml.py](https://codecov.io/gh/open-sdg/sdg-build/pull/336?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-sdg#diff-c2RnL3RyYW5zbGF0aW9ucy9UcmFuc2xhdGlvbklucHV0WWFtbC5weQ==) | `88.57% <100.00%> (+6.75%)` | :arrow_up: | | [sdg/open\_sdg.py](https://codecov.io/gh/open-sdg/sdg-build/pull/336?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-sdg#diff-c2RnL29wZW5fc2RnLnB5) | `79.18% <0.00%> (+0.55%)` | :arrow_up: | | [sdg/outputs/OutputSdmxMl.py](https://codecov.io/gh/open-sdg/sdg-build/pull/336?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-sdg#diff-c2RnL291dHB1dHMvT3V0cHV0U2RteE1sLnB5) | `76.92% <0.00%> (+3.07%)` | :arrow_up: | | [sdg/inputs/InputBase.py](https://codecov.io/gh/open-sdg/sdg-build/pull/336?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-sdg#diff-c2RnL2lucHV0cy9JbnB1dEJhc2UucHk=) | `87.05% <0.00%> (+5.43%)` | :arrow_up: | | [sdg/check\_csv.py](https://codecov.io/gh/open-sdg/sdg-build/pull/336?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-sdg#diff-c2RnL2NoZWNrX2Nzdi5weQ==) | `82.50% <0.00%> (+22.24%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-sdg). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-sdg)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

otis-bath commented 1 year ago

@brockfanning since we don't use yml for our metadata/indicator config is there a way we are able to test this?

brockfanning commented 1 year ago

@otis-bath You could try removing the translations section from your config_data.yml file, and then adding this:

inputs:
  - class: InputCsvData
    path_pattern: data/*.csv
  - class: InputYamlMeta
    path_pattern: indicator-config/*.yml
    git: false
  - class: InputYamlMdMeta
    path_pattern: meta/*.md
    git: true
    git_data_dir: data

translations:
  - class: TranslationInputSdgTranslations
    source: https://github.com/open-sdg/sdg-translations.git
    tag: 2.1.0
  - class: TranslationInputYaml
    source: translations

Then you could create two new files: indicator-config/1-1-1.yml and translations/en/foo.yml. The contents of both could the same snippet as above:

foo: 'hello 'open' sdg'
otis-bath commented 1 year ago

@brockfanning - thank you for this and am now receiving the error message

expected <block end>, but found '<scalar>'

Is this what it should be, I am not receiving the comprehensive error message shown in the files changed, you can see this message here https://github.com/ONSdigital/sdg-data/actions/runs/4345293662/jobs/7589796280

brockfanning commented 1 year ago

@otis-bath It's there, but you have to scroll up a bit. There are a few annoying warnings in there (warn(f"Print area cannot be set to Defined name: {defn.value}.")) that I would like to get rid of. Let me see if I can get rid of those.

brockfanning commented 1 year ago

@otis-bath Could you trigger it again? I've made a slight tweak to hopefully avoid those warnings.

otis-bath commented 1 year ago

@brockfanning refreshed - you can see the readout here https://github.com/ONSdigital/sdg-data/actions/runs/4345850379/jobs/7591124715

brockfanning commented 1 year ago

@otis-bath Thanks - I believe I've got it now so that the error message should show up at the very bottom. Could you trigger it again?