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 #71, Moves interface definition files to inc #72

Merged
merged 1 commit into from
Dec 22, 2022
Merged

Conversation

chillfig
Copy link
Contributor

@chillfig chillfig commented Dec 6, 2022

Checklist (Please check before submitting)

Describe the contribution

Testing performed
make install
lcov

Expected behavior changes
No expected behavior change.

System(s) tested on
-Ubuntu 18.04

Additional context
N/A

Third party code
If included, identify any third party code and provide text file of license

Contributor Info - All information REQUIRED for consideration of pull request
-Justin Figueroa, Vantage

@chillfig chillfig self-assigned this Dec 6, 2022
@chillfig chillfig force-pushed the inc branch 2 times, most recently from e7dd5dc to 18d60fa Compare December 6, 2022 23:23
@chillfig chillfig changed the title Fix #71, Move interface definition files to inc Fix #71, Moves interface definition files to inc Dec 6, 2022
Copy link
Contributor

@jphickey jphickey left a comment

Choose a reason for hiding this comment

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

Generally looks good to me, just one minor comment noted, but that does not really affect the fundamentals at all.

target_include_directories(coverage-ds_internal-stubs PUBLIC utilities)
target_include_directories(coverage-ds_internal-stubs PUBLIC ../fsw/inc)
target_include_directories(coverage-ds_internal-stubs PUBLIC ../fsw/src)
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nitpick here, these actually do not need to be separate calls to target_include_directories ... they can be all listed in one call. Furthermore it might be more future-proof to reference the public includes via the ds target property, that is:

target_include_directories(coverage-ds_internal-stubs PUBLIC
    $<TARGET_PROPERTY:ds,INTERFACE_INCLUDE_DIRECTORIES>
)

This better follows the "DRY" principle - don't repeat yourself.... but it's not critical. Unfortunately that won't work for "src" because there is no property for that one).

I'd also suggest ditching the "utilities" subdirectory here - doesn't seem like it accomplishes anything except adding some build complexity, but that can be considered as a future enhancement, not related to this change.

@dzbaker dzbaker merged commit 1d84940 into nasa:main Dec 22, 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.

Move interface definition files to "inc" location
4 participants