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 repository #115

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 represents changes made in the code base and documentation, merging into the master branch. #114 addresses changes made in the paper, merging into https://github.com/CMakePP/CMakePPLang/pull/87 and not intended to merge into master.

  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:

Although 2 refers to the paper, this PR also partially addresses it by clarifying what CTOR means in the "Example Usage" section of README.rst. Comment 2 is primarily addressed in #114.

Comment 3 is addressed in #114.

To address comment 4, the version on https://cmakepp.github.io/CMakePPLang/ is a target version for release 1.0.0. It will need to start being updated with each release after the 1.0 release, which we plan to do with the Python module for git, using it to set the version off of the git tags in the docs/source/conf.py file.

This PR addresses 5 by fixing the incorrect link, https://github.com/CMakePP/cmakepp_lang, to the correct repository link, https://github.com/CMakePP/CMakePPLang. This seems to be a typo since cmakepp_lang was being used (correctly) everywhere around it.

TODOs

ryanmrichard commented 11 months ago

Consider scheduling a weekly build/test action with a badge in the README to indicate if the latest version is stable.

I would just make this an issue for now unless you have a really quick fix. I want to get the 1.0 release out as soon as possible, we can add new features after that.

zachcran commented 11 months ago

Consider scheduling a weekly build/test action with a badge in the README to indicate if the latest version is stable.

I would just make this an issue for now unless you have a really quick fix. I want to get the 1.0 release out as soon as possible, we can add new features after that.

This has been moved to issue #116 for now.

bast commented 11 months ago

Thanks for these changes!