Skip to content
This repository has been archived by the owner on Feb 2, 2023. It is now read-only.

Ensure values are str types when converting from YAML to Markdown #12

Merged
merged 2 commits into from
Jan 24, 2022

Conversation

mcdonnnj
Copy link
Member

@mcdonnnj mcdonnnj commented Jan 24, 2022

🗣 Description

This pull request updates portions of the yml2md command so that any values where we check the len() is checked for truthiness and converted with str() first. It also converts a piece of error handling from manually printing to stderr to instead use the logging library appropriately.

💭 Motivation and context

When a person who is unfamiliar with YAML is updating an entry they may see no problem with the following:

  affected_versions:
    - < 1.4
    - 1.5
    - '> 1.6'

However, the second value will be read as a float value instead of as a str value. This will cause the yml2md command to fail with a TypeError when processing this entry.

🧪 Testing

Automated tests pass. When run with the sample version data above the v1.1.1 branch fails with a TypeError as expected. When used with this branch the conversion completes with no issues.

✅ Pre-approval checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
  • These code changes follow cisagov code standards.
  • All new and existing tests pass.

Use the logging library to output messages when handling exceptions
instead of manually redirecting print to stderr.
Ensure that only truthy values that have been cast to str are passed to
len() checks when outputting rows. This will ensure that numeric values
are correctly handled if they were not stored as strings in the source
YAML file.
@mcdonnnj mcdonnnj added the bug This issue or pull request addresses broken functionality label Jan 24, 2022
@mcdonnnj mcdonnnj self-assigned this Jan 24, 2022
@mcdonnnj mcdonnnj requested a review from jsf9k as a code owner January 24, 2022 16:43
Copy link
Member

@dav3r dav3r left a comment

Choose a reason for hiding this comment

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

This makes sense to me. String the things. 🚌

@mcdonnnj mcdonnnj merged commit d21ed8d into v1.1.1 Jan 24, 2022
@mcdonnnj mcdonnnj deleted the improvement/enforce_string_conversion branch January 24, 2022 17:10
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1741031944

  • 0 of 2 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 29.459%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/ymlmd/yml2md.py 0 2 0.0%
Totals Coverage Status
Change from base Build 1740961890: 0.0%
Covered Lines: 103
Relevant Lines: 344

💛 - Coveralls

@mcdonnnj mcdonnnj mentioned this pull request Jan 24, 2022
8 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug This issue or pull request addresses broken functionality
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants