-
Notifications
You must be signed in to change notification settings - Fork 2
Update transform_view to exemplar-based INTERFACE configuration #10
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
Conversation
ac3f091 to
7bedbab
Compare
756dedc to
0f8f41d
Compare
0f8f41d to
f5507d4
Compare
|
|
||
| <details> | ||
| <summary> Use beman.transform_view directly from C++ </summary> | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The statement below is valid -- we really need to separate the readme for building versus 'using in your project'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me clarify what I'm talking about:
This library is header only. If you want to use `beman.transform_view` from your
project, you can include `beman/transform_view/*.hpp` files from your C++ source
files
For me this is a valid way to use the library -- especially if you're using a non-cmake build system. It's a very common way to use header only libraries because it's easy and portable. We shouldn't discourage it just because some of our core members don't want to consume libraries this way.
The more general issue is that the examplar 'readme outline' is a mess. In my view all this 'building for developmentsort of text shouldn't be mixed in with the 'how do I just use this in my project: aka conan, vcpkg, etc. I think @ednolan has mentioned that thebuilding for development` should probably be a linked .md file so that the primary markdown can be radically simplified and less overwhelming.
So we don't need to fix it here now -- but it's bugging me :)
tzlaine
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all seems fine to me. Are Jeff's comments blocking this? I'm unclear if I should merge it as-is.
JeffGarland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I merged the one thing that really needed changing -- the unconditional requirement of ctest even when not building tests.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
No description provided.