Submitting author: @eidheim (Ole Christian Eidheim)
Repository: https://gitlab.com/eidheim/Simple-Web-Server
Version: v3.0.1
Editor: @gkthiruvathukal
Reviewers: @ts-adi, @HaoZeke
Archive: 10.5281/zenodo.3358045
Status badge code:
HTML: <a href="http://joss.theoj.org/papers/093a37e7d698d3350b2d4e564dcfb69b"><img src="http://joss.theoj.org/papers/093a37e7d698d3350b2d4e564dcfb69b/status.svg"></a>
Markdown: [](http://joss.theoj.org/papers/093a37e7d698d3350b2d4e564dcfb69b)
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.)
@ts-adi and @HaoZeke, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @gkthiruvathukal know.
β¨ Please try and complete your review in the next two weeks β¨
paper.md
file include a list of authors with their affiliations?paper.md
file include a list of authors with their affiliations?Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @ts-adi it looks like you're currently assigned to review this paper :tada:.
:star: Important :star:
If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews πΏ
To fix this do the following two things:
For a list of things I can do to help you, just type:
@whedon commands
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
@whedon generate pdf
Attempting PDF compilation. Reticulating splines etc...
Hi, @eidheim I've initiated the review process and hope to finish in the next couple of days. I will be opening pull requests and issues on the repository tracker, but I might also ask for clarifications here (typically with matters pertaining to the paper itself). In addition, for the paper.md
, I request that a copy to make changes on hackmd.io is made available to me and @ts-adi since it is not appropriate to make changes to it via pull requests.
Also since this is my first review working with something on GitLab, which will not cross-reference (attached image makes this clearer) PRs and issues to this issue thread automatically, how would you prefer to handle the issues?
Perhaps @gkthiruvathukal might want to sound off on this as well? Should I manually link to the pull requests or issues on GitLab?
@HaoZeke @ts-adi Thank you for reviewing this paper, and thank you all for the comments already made in #1579.
I invited @HaoZeke and @gkthiruvathukal to comment on the paper on hackmd.io. I'm new to hackmd.io, and could not find a way to invite @ts-adi since I could not find an email address on your GitHub profile. @ts-adi you can mail me on [email protected], and I'll invite you to comment on hackmd.io as well.
With respect to GitLab cross-reference, I suspect manual links would be needed. Since you might be new to GitLab, please note that pull requests are called merge requests on GitLab, but the interface should otherwise hopefully be familiar.
I update paper.md based on feedback in https://github.com/openjournals/joss-reviews/issues/1579#issuecomment-514678457 in commit https://gitlab.com/eidheim/Simple-Web-Server/commit/5d89bbcd0fdcba3584fae60fd50dec5da9b6e202. I also updated the paper on hackmd.io since this change was suggested in the pre-review, but I guess going forward I should not continuously update the paper on hackmd.io?
@eidheim, actually it would be lengthier to keep discussing small issues pertaining to the paper on issues so my take is to usually just leave comments on hackmd instead for the paper.
Also @gkthiruvathukal I seem to be missing my checklist...
@whedon list reviewers
Here's the current list of reviewers: https://bit.ly/joss-reviewers
@whedon commands
Here are some things you can ask me to do:
# List all of Whedon's capabilities
@whedon commands
# Assign a GitHub user as the sole reviewer of this submission
@whedon assign @username as reviewer
# Add a GitHub user to the reviewers of this submission
@whedon add @username as reviewer
# Remove a GitHub user from the reviewers of this submission
@whedon remove @username as reviewer
# List of editor GitHub usernames
@whedon list editors
# List of reviewers together with programming language preferences and domain expertise
@whedon list reviewers
# Change editorial assignment
@whedon assign @username as editor
# Set the software archive DOI at the top of the issue e.g.
@whedon set 10.0000/zenodo.00000 as archive
# Set the software version at the top of the issue e.g.
@whedon set v1.0.1 as version
# Open the review issue
@whedon start review
EDITORIAL TASKS
# Compile the paper
@whedon generate pdf
# Compile the paper from alternative branch
@whedon generate pdf from branch custom-branch-name
# Remind an author or reviewer to return to a review after a
# certain period of time (supported units days and weeks)
@whedon remind @reviewer in 2 weeks
# Ask Whedon to accept the paper and deposit with Crossref
@whedon accept
# Ask Whedon to check the references for missing DOIs
@whedon check references
@ts-adi I know what I did wrong. I used @whedon assign
for both of you during pre-review. I am supposed to use @whedon add
for the second reviewer. Grr.
I hope I can get one of the @openjournals/joss-editors to help me add you.
As far as I understand, I can just manually edit the first whedon comment in this thread to add myself if that is acceptable, @gkthiruvathukal.
@whedon add @HaoZeke as reviewer
OK, @HaoZeke is now a reviewer
@HaoZeke you should be good to go now!
@eidheim While I am going through the paper and confirming the code functionality, could you please start putting doxygen
style comments in your code in the meantime? http://www.doxygen.nl/manual/docblocks.html
This will ensure that when I get to the documentation validation part, you're already close to being done :)
If you need any pointers, here and here are some .h
and .cpp
file that I consider well-documented
Also, my email is [email protected] for hackmd.io
@ts-adi To make the comments as compact as possible I use ///
instead (see for instance: https://gitlab.com/eidheim/Simple-Web-Server/blob/master/server_http.hpp#L334). This style is mentioned in http://www.doxygen.nl/manual/docblocks.html. But if you much prefer another style I can of course consider changing this:) Make note that while the public functions are documented, there might be private/protected functions that might not be documented.
Thank you, I have now added you to the hackmd.io document.
@eidheim I'm less concerned with how you denote the comment block, and more concerned with the content of the comment. I think the current API comments you have are a good start, but all of them should provide more details about the function (eg, explaining arguments, valid ranges, default values, return values, etc).
In the end, a user should be able to use your code (and have a baseline understanding of what it does) without digging through any code files.
@ts-adi Sure, I'll extend the documentation as you suggest. I'll try complete this before this time Tomorrow.
@ts-adi The documentation has now been improved in https://gitlab.com/eidheim/Simple-Web-Server/commit/ed46b43fa7ff56a605b35a50bb08649dea372928. Thank you again for the feedback so far.
I also added Contributing Guidelines in https://gitlab.com/eidheim/Simple-Web-Server/commit/852281f34713cf8ef60e7bb10d5a20652eaea88e
@eidheim Thanks for the changes. I am satisfied by the documentation level for the core-code, however, there are no clear links to a compiled form of the documentation. Perhaps a hosted version? Or even a link to the doxygen
document (pdf)? In fact, an instructive example might actually be using the server to locally serve the doxygen HTML docs as well.
Also I noticed that the tests have no output which might be a bit confusing for neophytes. Please add some indication of which test is being run.
@HaoZeke Thank you again for the feedback. I added documentation using GitLab pages in commit https://gitlab.com/eidheim/Simple-Web-Server/commit/dcc609067856617dc2b9eb237dc2ea5568e51f88.
Regarding the tests, the test source files are currently shown when running the tests:
$ make test
Test project Simple-Web-Server/build
Start 1: io_test
1/4 Test #1: io_test .......................... Passed 28.32 sec
Start 2: parse_test
2/4 Test #2: parse_test ....................... Passed 0.02 sec
Start 3: crypto_test
3/4 Test #3: crypto_test ...................... Passed 0.07 sec
Start 4: status_code_test
4/4 Test #4: status_code_test ................. Passed 0.01 sec
100% tests passed, 0 tests failed out of 4
Total Test time (real) = 28.43 sec
One alternative would be to divide the tests into a larger number of source files, but this would drastically increase the compilation time.
The second alternative is to add multiple test cases with textual descriptions in each source file, but this is not supported, to my knowledge, by CTest (the build tool included in CMake) that is currently used. One would therefore have to add another dependency, for instance Catch2, to be able achieve this. Although, I am hesitant to do this as it would introduce additional complexity to the Simple-Web-Server project which supports quite a few old and new platforms and compilers. Additionally, writing tests will be more cumbersome, as IDE C++ parsers like libclang struggle with code inside macros such as is needed to achieve test cases with textual description in for instance Catch2 (see writing-tests), Boost.Test (see Test driven development with Boost.Test) and Google Test (see sample1_unittest.cc).
@eidheim Thanks for making the changes to the documentation. As you can notice, we're making progress with the approval checklist.
Now as for the tests, normally assert
statements show some sort of "Assertion Failed" message if the condition fails. I am yet to run your tests at my end, but want to confirm if they behave in this manner. Your output in the last comment shows all test suites passing, which is the output I'd expect as well. If there is a failure, would there be a cerr
output for the condition? If so, then I would be satisfied with the testing suite.
@eidheim in the setup instructions in the readme, I would appreciate if you could list the apt packages that newbies need to install on a standard ubuntu box to satisfy the dependencies you listed. From what I see, these would be cmake
, libboost-all-dev
and libssl-dev
. In fact, to increase traction of your software among first-timers, maybe even list out the sudo apt install
command installing all these things.
@ts-adi Thank you again for your input.
Normally, with CTest, one would have to set the environment variable CTEST_OUTPUT_ON_FAILURE
to get detailed output on condition errors. For instance, if a coding error in SimpleWeb::Percent::encode()
is made:
$ CTEST_OUTPUT_ON_FAILURE=1 make test
Running tests...
Test project Simple-Web-Server/build
Start 1: io_test
1/4 Test #1: io_test .......................... Passed 28.00 sec
Start 2: parse_test
2/4 Test #2: parse_test .......................Child aborted***Exception: 0.01 sec
Assertion failed: (Percent::encode(percent_decoded) == percent_encoded), function main, file Simple-Web-Server/tests/parse_test.cpp, line 136.
Start 3: crypto_test
3/4 Test #3: crypto_test ...................... Passed 0.06 sec
Start 4: status_code_test
4/4 Test #4: status_code_test ................. Passed 0.00 sec
75% tests passed, 1 tests failed out of 4
Total Test time (real) = 28.08 sec
The following tests FAILED:
2 - parse_test (Child aborted)
Errors while running CTest
make: *** [test] Error 8
Note that CTEST_OUTPUT_ON_FAILURE
is set when running the CI jobs (see https://gitlab.com/eidheim/Simple-Web-Server/blob/master/.gitlab-ci.yml#L4).
Regarding dependency installation instructions, I added more detailed instructions in https://gitlab.com/eidheim/Simple-Web-Server/commit/99d14cf841ca34577431571e6ab84ad6795fa33b.
@eidheim thanks for being so responsive and patient.
I am more used to tests written with Catch2 which are more verbose, however, I am satisfied with the Ctest suite, especially given the rationale (regarding libclang). However, if possible I would like an example which is of more research-oriented use. For example, an algorithm like a queuing or a packet congestion management algorithm (or something showing the relative response times compared to say, the express server), to comply with the requirement of a real-world analysis problem being used as an example. The same could also be added to the paper.md
file. An example utilizing the ScopeRunner construct would be useful as it is mentioned specially in the write-up. Also, as per my understanding of the Chung and Callin thesis they used a fork, so it might be better to say it has been adapted for use in various projects as it is easily extensible.
I believe if these are fixed then from my side I would be happy to recommend this package for JOSS.
Regarding dependency installation instructions, I added more detailed instructions in https://gitlab.com/eidheim/Simple-Web-Server/commit/99d14cf841ca34577431571e6ab84ad6795fa33b.
@eidheim Thanks, this is exactly the crisp content I was looking for. However, can you please add this to your main readme? No need to have a separate dependency install markdown file in docs
. This would satisfy the Installation instructions in the checklist for me.
@HaoZeke Thank you again for the input.
Although not exactly what you asked for, I added a benchmark page here: https://gitlab.com/eidheim/Simple-Web-Server/blob/benchmarks/docs/benchmarks.md. It is not yet in the master branch, since it is just a draft so far. I am also a bit unsure if I should go in this direction, as I'm thinking benchmarking is out of scope of this paper, but at least you see how it would compare to Express and a web library written in Rust.
I also did some changes in paper.md based on your input: https://gitlab.com/eidheim/Simple-Web-Server/commit/8edf4355f254456ad10a296f3cb2b6168b14c349. Again, not entirely what you asked for, but I added a paragraph on the subject of performance, and added further details on the ScopeRunner class.
Regarding the Chung and Callin thesis, note that they link to an old version of the Simple-Web-Server project on GitHub (https://github.com/eidheim/Simple-Web-Server). The project was later moved to GitLab as mentioned in the old GitHub project.
@ts-adi I moved the dependency installation instructions to the main README.md in: https://gitlab.com/eidheim/Simple-Web-Server/commit/8c146111028aa82dce80b0d7da3069c2810ceea1. Thank you again.
@eidheim, thanks for the changes. The benchmarks are relevant, since the performance would be a major deciding factor. paper.md
also looks great. So I believe as soon as the benchmarks.md
file lands on master, I can recommend this.
@eidheim In my opinion, benchmarking is not relevant here as I don't see any performance claims made in your paper.md
. So I will mark it complete on my checklist but please do still satisfy the requirements set out by @HaoZeke.
My only remaining concern now is with _example usage_. The example files you've provided are great, but I'd ask you to explain a little more in your README.md
as to what is happening when the example scripts are run (what calls are being made, what output is expected, what are the different things that can be tested). The code is fine; you simply need to explain it a bit more under the Usage section.
@HaoZeke @ts-adi Thank you again for great feedback.
@HaoZeke I have now pushed the benchmark page to master: https://gitlab.com/eidheim/Simple-Web-Server/commit/5b6a606f4cd844c5966caeaa73c31858e18e57a1
@ts-adi The usage section is now extended, and the examples have additional output: https://gitlab.com/eidheim/Simple-Web-Server/commit/0eeb972b51e3d6bb58a4fdd5487d8900b89af14c
@whedon check references
Attempting to check references...
```Reference check summary:
OK DOIs
MISSING DOIs
INVALID DOIs
@eidheim Thanks for your patience in handling all the issues raised by me and @ts-adi. The readme
changes are very well done. I believe the DOI @whedon seems to think is missing is incorrect. The paper.bib
file is correctly formatted (including the boost citation). @gkthiruvathukal, my review is complete and I recommend this for publication.
@eidheim I think if you put the doi
for the "Boost C++ Libraries" entry, @whedon should be good with it. While you're at it, I think it's a good idea that you look at all the references mentioned in your paper.bib
and see which have a valid DOI. Add it in for all the ones you find a DOI for.
@HaoZeke @ts-adi I agree that https://doi.org/10.1007/978-1-4419-7719-9_6 is incorrect, it seems to be a book that includes a chapter about the Boost C++ libraries.
@ts-adi I could only find a valid DOI for the websocket protocol, which I added in https://gitlab.com/eidheim/Simple-Web-Server/commit/db24254c96225c65af4637d387eb638e95ed313d.
@eidheim ah, I see. You weren't actually trying to quote that chapter from the book, but just reference the Boost project in general.
Cool, in that case, I'm done with my review. @gkthiruvathukal this paper has my recommendation.
Based on the thread, it seems like both @ts-adi and @HaoZeke are ready to proceed. Therefore, I am ready to proceed, too.
@eidheim, Can you please do the following?
If you've already done these, just go ahead and check the boxes. In your follow-up, just include the tagged release and Zenodo DOI so I can set these using whedon.
Thank you @gkthiruvathukal .
@whedon check references
Attempting to check references...
```Reference check summary:
OK DOIs
MISSING DOIs
INVALID DOIs
@whedon commands
Here are some things you can ask me to do:
# List all of Whedon's capabilities
@whedon commands
# Assign a GitHub user as the sole reviewer of this submission
@whedon assign @username as reviewer
# Add a GitHub user to the reviewers of this submission
@whedon add @username as reviewer
# Remove a GitHub user from the reviewers of this submission
@whedon remove @username as reviewer
# List of editor GitHub usernames
@whedon list editors
# List of reviewers together with programming language preferences and domain expertise
@whedon list reviewers
# Change editorial assignment
@whedon assign @username as editor
# Set the software archive DOI at the top of the issue e.g.
@whedon set 10.0000/zenodo.00000 as archive
# Set the software version at the top of the issue e.g.
@whedon set v1.0.1 as version
# Open the review issue
@whedon start review
EDITORIAL TASKS
# Compile the paper
@whedon generate pdf
# Compile the paper from alternative branch
@whedon generate pdf from branch custom-branch-name
# Remind an author or reviewer to return to a review after a
# certain period of time (supported units days and weeks)
@whedon remind @reviewer in 2 weeks
# Ask Whedon to accept the paper and deposit with Crossref
@whedon accept
# Ask Whedon to check the references for missing DOIs
@whedon check references
@whedon set 10.5281/zenodo.3358045 as archive
OK. 10.5281/zenodo.3358045 is the archive.
@whedon set v3.0.1 as version
OK. v3.0.1 is the version.
@eidheim The one thing I am seeing is the missing DOI issue for Boost. I've gone ahead and set the archive and version as these won't change if you correct that issue.
@openjournals/joss-editors Pending the one issue with DOIs (which may require your help as well) I am ready to recommend acceptance of this submission.
@gkthiruvathukal The reported missing DOI is incorrect. Please see previous comment: https://github.com/openjournals/joss-reviews/issues/1592#issuecomment-517176440.
@eidheim That is one of the reasons I wrote what I did above. I am pulling in the editors to have them take a look. This must be some sort of parsing issue, etc. One thing you could try is to remove the http + hostname from the DOI. If you look at how i set the DOI for your paper, it starts with the numeric portion of the identifier.
@eidheim @gkthiruvathukal the doi
fields of the .bib file should only have the DOIs themselvesβplease remove https://doi.org/
@kyleniemeyer Thank you, but that is not the issue I think. There is no https://doi.org/
in the .bib file (https://gitlab.com/eidheim/Simple-Web-Server/blob/master/paper/paper.bib).
@eidheim oh, I misunderstood the issue (and didn't read the prior comments above). You cited the boost libraries directly, and whedon found something with a DOI that has the same name.
Unless you intend to cite the book chapter (perhaps in addition to the software itself, but this isn't necessary), then we can safely ignore this.
whedon's DOI messages are not something we have to resolve, they are just meant to be suggestions and to find any glaring issues.
@kyleniemeyer Thank you again. @gkthiruvathukal then we can safely ignore the output from MISSING DOIs
as I do not wish to cite the book chapter mentioned.
@whedon check references
Attempting to check references...
```Reference check summary:
OK DOIs
MISSING DOIs
INVALID DOIs
@eidheim I think you still need to update the DOI. See @kyleniemeyer's comment above. If you wish to remove the citation, this is also ok, but it is still present in your list of references.
Thank you @gkthiruvathukal . I removed the boost citation in https://gitlab.com/eidheim/Simple-Web-Server/commit/3dd3f1d4be4e0f84c1f9f532c3049e2c5e1e94cc.
Do you wish me to create a new and updated repository release/tag and/or zenodo archive?
@eidhelm, you're welcome. I don't believe you need to update the release or archive, since these both reflect the version we're accepting.
@openjournals/joss-editors, I recommend accepting this submission, pending any final input you may have.
@gkthiruvathukal You want to use @openjournals/joss-eics here!
Thanks, @kthyng, my bad. A little too fast with tab completion. Thanks for catching that!
@openjournals/joss-eics, I recommend accepting this submission, pending any final input you may have.
@whedon check references
Attempting to check references...
```Reference check summary:
OK DOIs
MISSING DOIs
INVALID DOIs
@whedon generate pdf
Attempting PDF compilation. Reticulating splines etc...
Hi @eidheim, could you merge this PR with an edit to a reference: https://gitlab.com/eidheim/Simple-Web-Server/merge_requests/244
@kyleniemeyer your PR is now merged. Thank you.
@whedon generate pdf
Attempting PDF compilation. Reticulating splines etc...
@whedon accept
Attempting dry run of processing paper acceptance...
Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/927
If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/927, then you can now move forward with accepting the submission by compiling again with the flag deposit=true
e.g.
@whedon accept deposit=true
@whedon accept deposit=true
Doing it live! Attempting automated processing of paper acceptance...
π¦π¦π¦ π Tweet for this paper π π¦π¦π¦
π¨π¨π¨ THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! π¨π¨π¨
Here's what you must now do:
Party like you just published a paper! πππ¦ππ»π€
Any issues? notify your editorial technical team...
Congrats on your article's publication in JOSS @eidheim! Many thanks to @gkthiruvathukal for editing, and @ts-adi and @HaoZeke for reviewing, this submission.
:tada::tada::tada: Congratulations on your paper acceptance! :tada::tada::tada:
If you would like to include a link to your paper from your README use the following code snippets:
Markdown:
[](https://doi.org/10.21105/joss.01592)
HTML:
<a style="border-width:0" href="https://doi.org/10.21105/joss.01592">
<img src="https://joss.theoj.org/papers/10.21105/joss.01592/status.svg" alt="DOI badge" >
</a>
reStructuredText:
.. image:: https://joss.theoj.org/papers/10.21105/joss.01592/status.svg
:target: https://doi.org/10.21105/joss.01592
This is how it will look in your documentation:
We need your help!
Journal of Open Source Software is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:
Most helpful comment
@HaoZeke you should be good to go now!