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

Fix bug in the convert-cisagov command #9

Merged
merged 1 commit into from
Jan 20, 2022

Conversation

mcdonnnj
Copy link
Member

@mcdonnnj mcdonnnj commented Jan 20, 2022

🗣 Description

This pull request fixes a bug in the convert-cisagov command's functionality that resulted in rows being dropped while grouping. Additionally debug output was added to provide good indicators on things processing correctly if needed.

💭 Motivation and context

This bug resulted in row from the Markdown file being read in getting dropped when using itertool.groupby(). The root cause is that the source list needs to be sorted using the same logic that is used to group elements.

🧪 Testing

Automated tests pass. I confirmed with the new debug output that the same number of rows that were read in were in the grouped data and that output files looked appropriate.

✅ 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.

The itertools.groupby() function only correctly functions if the list
it is grouping is pre-sorted in the same manner as the grouping key. If
the list is not similarly sorted it will result in lost list items. We
also switch to using `.lower()` instead of `.upper()` to be consistent
with other sorting in this project.
@mcdonnnj mcdonnnj added bug This issue or pull request addresses broken functionality improvement This issue or pull request will add new or improve existing functionality labels Jan 20, 2022
@mcdonnnj mcdonnnj self-assigned this Jan 20, 2022
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1725465369

  • 0 of 7 (0.0%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.5%) to 29.637%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/mdyml/convert_cisagov.py 0 7 0.0%
Files with Coverage Reduction New Missed Lines %
src/mdyml/convert_cisagov.py 2 19.78%
Totals Coverage Status
Change from base Build 1725072938: -0.5%
Covered Lines: 103
Relevant Lines: 343

💛 - Coveralls

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.

LGTM - good call on the extra debugging output. 👍

Copy link
Member

@jsf9k jsf9k left a comment

Choose a reason for hiding this comment

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

Nice detective work!

@mcdonnnj mcdonnnj merged commit e1c5d47 into v1.1.1 Jan 20, 2022
@mcdonnnj mcdonnnj deleted the bug/fix_cisagov_conversion branch January 20, 2022 21:56
@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 improvement This issue or pull request will add new or improve existing functionality
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants