search

CMakePP / CMakePPLang

Object-oriented extension to the CMake language.
http://cmakepp.github.io/CMakePPLang/
Apache License 2.0
11 stars 4 forks source link

Addressing comments from bast on the paper #114

Closed zachcran closed 11 months ago

zachcran commented 11 months ago

Is this pull request associated with an issue(s)? Please list issues associated with this pull request, including closing words as appropriate.

https://github.com/openjournals/joss-reviews/issues/5711

Description

Reviewer @bast has raised some concerns and comments on CMakePPLang and the paper. This PR addresses changes made in the paper, merging into https://github.com/CMakePP/CMakePPLang/pull/87 and not intended to merge into master. #115 addresses changes made in the code base and documentation, merging into the master branch.

  1. The strategy for managing versioning, compatibility, deprecation, and API changes in this tool is not clear in the paper or code documentation.
  2. It is not clear what CTOR means in Figures 2 and 3 in the paper.
  3. In the paper, a short example showing the advantage of CMakePPLang's strong typing over CMake's weak typing would be useful.
  4. The CMakePPLang version listed on the documentation website, https://cmakepp.github.io/CMakePPLang/, shows 1.0.0, which is not the current version of CMakePPLang that is released.
  5. The install instructions have an incorrect link to the CMakePPLang repository.

This PR partially addresses 1 by adding a short statement explaining that CMakePPLang is versioned separately from CMake and that CMakePPLang uses semantic versioning. A more full discussion of versioning concerns are addressed in the documentation by changes made in #115.

This PR addresses 2 by clarifying in Figure 2 that "CTOR" is shorthand for "constructor".

This PR addresses 3 by adding an example comparing the various (mostly incorrect) ways that list(LENGTH can be called from CMake vs. how the strong typing of CMakePPLang can be used to write a wrapper that clarifies the ambiguity of the list(LENGTH signature and throw errors when the wrong arguments are used.

Comment 4 is addressed in #115.

Comment 5 is addressed in #115.

TODOs

bast commented 11 months ago

Thanks for these changes!