-
Notifications
You must be signed in to change notification settings - Fork 201
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
Deprecate multiple "success" code responses #483
Comments
Some discussion of this topic in the context of #846 with respect to My take on the topic is that all API functions should strive to meet the "ACID" test that is often employed in database software, described here: https://en.wikipedia.org/wiki/ACID In particular the atomicity principle (A) - A function either succeeds completely or fails completely. Applied to something like If it is decided that it is acceptable to store only a partial log message, then If it is decided that it is not acceptable to store only a partial log message, then This way it either succeeds completely or does nothing - there should be no half-done/middle-ground/gray area where it did something but not the whole thing. |
I went ahead and documented all the cases of extraneous CFE "success" status/information codes. Most offenders are in TBL, a few in ES. Here is my list along with a synopsis. CFE_ES_CDS_ALREADY_EXISTS: CFE_ES_LIB_ALREADY_LOADED: CFE_ES_ERR_SYS_LOG_TRUNCATED: CFE_TBL_INFO_UPDATE_PENDING: CFE_TBL_WARN_DUPLICATE: CFE_TBL_INFO_UPDATED: CFE_TBL_WARN_PARTIAL_LOAD: CFE_TBL_INFO_NO_UPDATE_PENDING: CFE_TBL_INFO_TABLE_LOCKED: CFE_TBL_INFO_NO_VALIDATION_PENDING: CFE_TBL_WARN_NOT_CRITICAL: CFE_TBL_INFO_RECOVERED_TBL: Edit - corrected assessment CFE_TBL_INFO_NO_UPDATE_PENDING and CFE_TBL_WARN_NOT_CRITICAL |
Also - noting that many offenses were in
So - it would be a wise investment to break this behemoth function up into more manageable parts. |
Thanks for the thorough analysis! |
Excellent analysis, thanks Joe! Should we denote codes that will be deprecated or otherwise have their semantics changed? I'm thinking in |
Yes, we've also been tracking cyclomatic complexity via CodeSonar reports. Not a priority for certification, but definitely on the list for a refactor. I doubt a big refactor will fit into the 7.0.0 dev cycle, so likely a target for next cycle unless it's done via contributions and even then timing is tight. |
Is your feature request related to a problem? Please describe.
API's with multiple "success" codes are frequently mishandled
Describe the solution you'd like
Single success response, unique information should be passed back in parameters
Describe alternatives you've considered
N/A
Additional context
N/A
Requester Info
Jacob Hageman - NASA/GSFC
The text was updated successfully, but these errors were encountered: