openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
714 stars 38 forks source link

[REVIEW]: CMakePPLang: An object-oriented extension to CMake #5711

Closed editorialbot closed 1 year ago

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@ryanmrichard<!--end-author-handle-- (Ryan M. Richard) Repository: https://github.com/CMakePP/CMakePPLang/ Branch with paper.md (empty if default branch): joss_paper Version: v1.0.3 Editor: !--editor-->@diehlpk<!--end-editor-- Reviewers: @bast, @tianyi93 Archive: 10.5281/zenodo.8339121

Status

status

Status badge code:

HTML: <a href="https://joss.theoj.org/papers/3cc2bc54aa95fc4c57aed8ccab049cae"><img src="https://joss.theoj.org/papers/3cc2bc54aa95fc4c57aed8ccab049cae/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/3cc2bc54aa95fc4c57aed8ccab049cae/status.svg)](https://joss.theoj.org/papers/3cc2bc54aa95fc4c57aed8ccab049cae)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@bast & @tianyi93, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @diehlpk know.

Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest

Checklists

📝 Checklist for @tianyi93

editorialbot commented 1 year ago

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

For a list of things I can do to help you, just type:

@editorialbot commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@editorialbot generate pdf
editorialbot commented 1 year ago
Software report:

github.com/AlDanial/cloc v 1.88  T=0.08 s (2856.9 files/s, 223586.4 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
CMake                          192           2269           4595           7572
reStructuredText                25            719           1011           1248
Markdown                         3             37              0            168
YAML                             8             32            138            136
TeX                              1             12              0            134
Python                           1              8             17             32
make                             1              4              6             10
Bourne Shell                     1              2              0              7
-------------------------------------------------------------------------------
SUM:                           232           3083           5767           9307
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 1 year ago

Wordcount for paper.md is 1172

editorialbot commented 1 year ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1063/5.0147903 is OK
- 10.1109/MC.2006.20 is OK

MISSING DOIs

- None

INVALID DOIs

- None
editorialbot commented 1 year ago

:warning: An error happened when generating the pdf.

ryanmrichard commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

diehlpk commented 1 year ago

Reminder set for @tianyi93 in 1 weeks

diehlpk commented 1 year ago

@editorialbot commands

editorialbot commented 1 year ago

Hello @diehlpk, here are the things you can ask me to do:


# List all available commands
@editorialbot commands

# Add to this issue's reviewers list
@editorialbot add @username as reviewer

# Remove from this issue's reviewers list
@editorialbot remove @username from reviewers

# Get a list of all editors's GitHub handles
@editorialbot list editors

# Assign a user as the editor of this submission
@editorialbot assign @username as editor

# Remove the editor assigned to this submission
@editorialbot remove editor

# Remind an author, a reviewer or the editor to return to a review after a 
# certain period of time (supported units days and weeks)
@editorialbot remind @reviewer in 2 weeks

# Check the references of the paper for missing DOIs
@editorialbot check references

# Perform checks on the repository
@editorialbot check repository

# Adds a checklist for the reviewer using this command
@editorialbot generate my checklist

# Set a value for version
@editorialbot set v1.0.0 as version

# Set a value for branch
@editorialbot set joss-paper as branch

# Set a value for repository
@editorialbot set https://github.com/organization/repo as repository

# Set a value for the archive DOI
@editorialbot set set 10.5281/zenodo.6861996 as archive

# Mention the EiCs for the correct track
@editorialbot ping track-eic

# Generates the pdf paper
@editorialbot generate pdf

# Recommends the submission for acceptance
@editorialbot recommend-accept

# Generates a LaTeX preprint file
@editorialbot generate preprint

# Flag submission with questionable scope
@editorialbot query scope

# Get a link to the complete list of reviewers
@editorialbot list reviewers

# Creates a post-review checklist with editor and authors tasks
@editorialbot create post-review checklist

# Open the review issue
@editorialbot start review
diehlpk commented 1 year ago

@editorialbot remind @bast in 1 weeks

editorialbot commented 1 year ago

Reminder set for @bast in 1 weeks

bast commented 1 year ago

@editorialbot remind @bast in 1 weeks

Thanks! I plan to do the review work coming week after Wednesday.

editorialbot commented 1 year ago

:wave: @bast, please update us on how your review is going (this is an automated reminder).

tianyizhangcs commented 1 year ago

Review checklist for @tianyi93

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

tianyizhangcs commented 1 year ago

The document is about CMakePPLang, an object-oriented extension to the CMake language. It aims to simplify the creation and maintenance of projects built on CMake. The author has provided a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience. It also compared with CMake++ and explained the need for CMakePPLang.

Few comments are listed below:

ryanmrichard commented 1 year ago

@tianyi93 when you get a chance please take a look at CMakePP/CMakePPLang#113. I believe it addresses your comments (and if it does not, please let me know).

bast commented 1 year ago

I am finally working on it - tracking my progress in this checklist:

Review checklist for @bast

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

bast commented 1 year ago

I still need to carefully go through my checklist above but for me the main question to address in the code documentation and in the paper is the question how the authors envision to manage versioning, compatibility, deprecation, and API changes in this tool. This tool is built on top of CMake language. The CMake language evolves (with its own versioning and deprectation mechanisms) and also CMakePPLang will evolve. How will it "move" with respect to an evolving CMake language underneath? If I as user had to decide whether to build my project on top of CMakePPLang, that would be a very important question for me. I would like to know what to expect 2-5 years down the line when CMake maybe has a new major version. Would my project stop working or would I have a way to pin a version? Do I need to anticipate lots of work updating my CMake code?

bast commented 1 year ago

When reading the paper alone and looking at Figures 2 and 3 I was asking myself what "CTOR" meant. In the code documentation this is clarified but the figures would be easier to read if "CTOR" was explained at least in the code comment inside the example code ("GET" and "SET" are on the other hand intuitively clear).

bast commented 1 year ago

A convincing argument to try this tool is its stronger typing. As a suggestion, it could be powerful to show in the paper a short example where because of weak typing a native CMake code could lead to an error that is difficult to catch whereas CMakePPLang catches the mistake with a clear error message since it is stricter about typing.

bast commented 1 year ago

Version on https://cmakepp.github.io/CMakePPLang/ seems to be 1.0.0 but I am not sure this matches the version of the code. But maybe it is the target version?

diehlpk commented 1 year ago

@ryanmrichard can you please have a look at bast's comments?

ryanmrichard commented 1 year ago

@diehlpk I have read his comments and @zachcran and I are working on addressing them. We're hoping to have addressed them by the end of the week.

bast commented 1 year ago

Thanks! I am working on the rest of the checklist so the missing checks do not mean that there is a problem.

bast commented 1 year ago

Install instructions here seem to point to a private or old repo:

- GIT_REPOSITORY https://github.com/CMakePP/cmakepp_lang
+ GIT_REPOSITORY https://github.com/CMakePP/cmakePPlang
tianyizhangcs commented 1 year ago

@tianyi93 when you get a chance please take a look at CMakePP/CMakePPLang#113. I believe it addresses your comments (and if it does not, please let me know).

LGTM, thanks!

diehlpk commented 1 year ago

@tianyi93 when you get a chance please take a look at CMakePP/CMakePPLang#113. I believe it addresses your comments (and if it does not, please let me know).

LGTM, thanks!

@tianyi93 can you please update the check list, if the author satisfied your comments.

bast commented 1 year ago

My comments have been addressed in https://github.com/CMakePP/CMakePPLang/pull/114 and https://github.com/CMakePP/CMakePPLang/pull/115 - thank you so much! After the changes are merged, can we build a new version of the manuscript for a final read-through? (If this is not the way how it usually works, I can also build it then locally)

ryanmrichard commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

bast commented 1 year ago

Thanks! All my points were carefully addressed.

tianyizhangcs commented 1 year ago

@tianyi93 when you get a chance please take a look at CMakePP/CMakePPLang#113. I believe it addresses your comments (and if it does not, please let me know).

LGTM, thanks!

@tianyi93 can you please update the check list, if the author satisfied your comments.

Updated, Thanks!

diehlpk commented 1 year ago

@editorialbot check references

diehlpk commented 1 year ago

@editorialbot generate paper

editorialbot commented 1 year ago

I'm sorry human, I don't understand that. You can see what commands I support by typing:

@editorialbot commands

editorialbot commented 1 year ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1063/5.0147903 is OK
- 10.1109/MC.2006.20 is OK

MISSING DOIs

- None

INVALID DOIs

- None
diehlpk commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

diehlpk commented 1 year ago

@ryanmrichard I read the paper, and the references are inconsistent. Sometimes there is a space word (reference) and sometimes there is no space word(reference). I think both are fine, but it should be consistent.

 (Richard et al., 2023) Better utilities and extensions in CMake can
26 help alleviate these issues, but these tools must be able to be designed in a maintainable and
27 testable way. 

Would it be possible to place the reference somewhere else? it is strange to start a sentence with the reference.

diehlpk commented 1 year ago

@ryanmrichard The C++ in the paper looks strange since the ++ are very big. One possible solution is to use C\texttt{++} to make it look nicer. This is not necessary, but I think the paper looks nicer.

ryanmrichard commented 1 year ago

@diehlpk Thanks for pointing those issues out. I agree with both of your comments. I looked into the C++ thing a bit and apparently ISO C++ has their own recommendation; I went with the ISO recommendation and think it looks nice.

@editorialbot generate pdf

diehlpk commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

diehlpk commented 1 year ago

@ryanmrichard Thanks, now I am OK with the paper.

Please generate the following:

ryanmrichard commented 1 year ago

The version number of CMakePPLang is v1.0.0

The Zenodo DOI is 10.5281/zenodo.8339016

diehlpk commented 1 year ago

@editorialbot set v1.0.0 as version

editorialbot commented 1 year ago

Done! version is now v1.0.0

diehlpk commented 1 year ago

@editorialbot set 10.5281/zenodo.8339016 as archive

editorialbot commented 1 year ago

Done! archive is now 10.5281/zenodo.8339016