Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #7, consolidate functions for managing tables #48

Merged
merged 1 commit into from
Sep 1, 2022

Conversation

havencarlson
Copy link
Contributor

Checklist (Please check before submitting)

Describe the contribution
Fix #7, consolidate SC_ManageAtsTable(), SC_ManageRtsTable(), and SC_ManageAppendTable()

Testing performed
Unit testing

Expected behavior changes
no impact to behavior

System(s) tested on

  • OS: Ubuntu 18.04

Contributor Info - All information REQUIRED for consideration of pull request
Haven Carlson - NASA

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Found 18 potential problems in the proposed changes. Check the Files changed tab for more details.

@havencarlson havencarlson requested a review from skliper September 1, 2022 14:29
Copy link
Contributor

@skliper skliper left a comment

Choose a reason for hiding this comment

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

Looks fine to me from a quick skim. Recommend adding someone from the core cFS team to review in detail. Could some of the related coverage tests be simplified based on the consolidation (no need to cover shared code 3 different times)? Good to show it still passes the original tests though! Maybe simplify UT's as a follow on issue (if it can actually be simplified)?

@dzbaker dzbaker merged commit 067714e into nasa:main Sep 1, 2022
@dmknutsen dmknutsen added this to the Draco milestone Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consolidate similar functions
4 participants