search

stachenov / quazip

Qt/C++ wrapper over minizip
Other
582 stars 232 forks source link

Replace `QUAZIP_ENABLE_TESTS` local variable with `option` #130

Closed ghost closed 3 years ago

ghost commented 3 years ago

Motivation

The user of the library should be able to turn QUAZIP_ENABLE_TESTS off.

Solution

We can do it by making an option of QUAZIP_ENABLE_TESTS. In addition, QUAZIP_ENABLE_TESTS shouldn't be set from the inside, thus setting it to ON statements are removed.

Now to build QuaZIP with tests just pass -DQUAZIP_ENABLE_TESTS=ON to CMake configure, or if you want to do it as a part of the project, then use set(QUAZIP_ENABLE_TESTS ON).

Resolves #119.

stachenov commented 3 years ago

Looks fine to me, but I'm curious why the tests are disabled by default? Is it a common practice? I'm fine either way, but I'm usually reluctant to introduce incompatible changes. The tests were enabled by default in the previous versions, so I'm not sure it's a good idea to change existing behavior with no good reason.

On the other hand, if we make it ON by default, then we'll break builds on systems that lack QtTest for whatever reason? Then maybe it's a sensible default to leave it OFF.

ghost commented 3 years ago

Looks fine to me, but I'm curious why the tests are disabled by default? Is it a common practice?

Yes, people tend to disable tests by default, because it's usually expected that the user of the library doesn't need them at all, it's just for the developer. Take a look at the spdlog library for example.

stachenov commented 3 years ago

Back in the ./configure age, it was the opposite: the user was expected to make check after a successful build, to ensure that the library works as intended on their specific systems with specific build settings.

But since nothing stops the user from building the tests manually anyway, I guess it doesn't matter much. Plus, when a project is used as a subdirectory, building tests is probably more of a nuisance.

All right, I'm merging it as it is.