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

add CMake option to toggle doxygen documentation #200

Merged
merged 1 commit into from
Aug 31, 2015
Merged

add CMake option to toggle doxygen documentation #200

merged 1 commit into from
Aug 31, 2015

Conversation

maddinat0r
Copy link
Contributor

Yet another PR, this time with the ability to toggle creation of doxygen documentation (on by default).

By the way, keep up the good work! 👍

@vitaut
Copy link
Contributor

vitaut commented Aug 30, 2015

Thanks for the PR, but what is the purpose of this option? The documentation is not generated by CMake, but only when you build the doc target which is not built by default.

@maddinat0r
Copy link
Contributor Author

You are right, but this might (in my case) confuse people when they compile a project (which depends on cppformat), since the main project will have a target named "doc". This target however will generate cppformat documentation, not the main project's documentation.
I could also push another commit to mark this option as advanced, so that it's invisible by default for the "normal" user (people who directly setup and compile cppformat as main project, instead of creating CMake dependencies).

vitaut added a commit that referenced this pull request Aug 31, 2015
add CMake option to toggle doxygen documentation
@vitaut vitaut merged commit 6de8f4a into fmtlib:master Aug 31, 2015
@vitaut
Copy link
Contributor

vitaut commented Aug 31, 2015

Fair enough. I think there is no need to mark the option advanced as the number of options is small, but I've changed the naming of this option and FMT_TESTS to be consistent with the names of the targets they controls, FMT_DOC for the doc target and FMT_TEST for the test target (75fdfe3). FMT_INSTALL is already in this form. Hope you don't mind.

@maddinat0r
Copy link
Contributor Author

Sounds logical to me. Thanks for your work! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants