JOSS review 2558: first set of comments
arcuri82 opened this issue · 4 comments
First set of comments for JOSS submission 2558.
PAPER:
The paper looks fine. I only have a few minor comments.
-
Summary: provide URL to original Bioclipse, besides tha academic reference to (Spjuth et al., 2007). Furthermore, should add 2-3 sentences specifying what Bioclipse does.
-
What is "Spring approaches"? Are you referring to the Spring Framework (https://spring.io/)?
-
Why there is the section for "Grabbing Bacting from Groovy"? Isn't that something more fitting for the Readme in repository instead of the paper? Furthermore, if you discuss it for Groovy, why not for JavaScript and Python as well?
README
-
"Bacting := acting as the Bioclipse TNG": what is TNG?
-
"If you use this software, please cite the Bioclipse 2 paper"
But this tool is called Bacting.
What is the relation between Bioclipse 2 and Bacting. Did you just re-named Bioclipse 2 into Bacting? If so, this should be clarified. -
"Bacting is a Java-based, open-source platform..." however, the example is in Groovy. Why not Java? And what is that example doing? (Could add 1-2 sentences saying what it should be for)
-
In the Readme, need a full working example to show what Bacting can do. I see the "For a description of the API, I refer to the book", but you still need some full examples here.
Note that JOSS explicitly requires: "Do the authors include examples of how to use the software (ideally to solve real-world analysis problems)." -
"All code in the /bioclipse/ folder": I do not see such folder.
CODE:
- "mvn clean install -Dgpg.skip" fails the build, due to
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-javadoc-plugin:3.2.0:jar (attach-javadocs) on project managers-ui: MavenReportException: Error while generating Javadoc:
[ERROR] Exit code: 1 - /Users/foo/WEB/joss/bacting/managers-core/src/main/java/net/bioclipse/managers/BioclipseManager.java:89: warning: no description for @throws
[ERROR] * @throws BioclipseException
as it is now, I cannot run the code, as I cannot compile it, nor follow an example to try it out.
-
The groupId is "io.github.egonw.bacting", which is the package used in some modules, e.g. "bacting-core". However, most of the code is under "net.bioclipse.managers".
Why? Shouldn't it be "io.github.egonw.bacting.managers"?
Or is this related to "All Bacting scripts will be backwards compatible with Bioclipse"?
That would make sense for Groovy.
But, in such case, what about Python and JavaScript? Or are those not supported in Bacting? This needs to be clarified in the documentation. -
There are some test cases, but I do not see any check on coverage. Could add JaCoCo (https://www.eclemma.org/jacoco/), and then publish the results on some services like CodeCov (https://codecov.io/). This can then be automated in Travis with a simple:
after_success:
- bash <(curl -s https://codecov.io/bash)
You can then also add a badge in Readme for GitHub to display (eg, as you already do for "build-passing" for Travis).
Thanks for your thorough review. There is a good bit of comments that I can accommodate; thank you!
Regarding the Groovy, yes, reviewing source code in a programming language you do not used before is tricky, indeed. Any Java is basically valid Groovy, but Groovy just removes a lot of boiler plate code. But I think I can make a Java example.
Similarly, thank you for challenging me to get it working in Jython and one of the Java JavaScript implementation (as it was used in Bioclipse).
These are just some quick notes as thank you for your review. I will follow up with a full rebuttal and list of updates.
Hi all, I am sorry I could not earlier get back to this. I'm picking these things up now. I will try to get everything done before January 4. I'll keep you posted.
@arcuri82, you asked "Why there is the section for "Grabbing Bacting from Groovy"?". I have this section in here to show the difference with the original Bioclipse script. I prefer to keep it.
You also wrote: "If you use this software, please cite the Bioclipse 2 paper, But this tool is called Bacting." Correct, but since the Bacting paper is not published yet, people cannot easily cite it. Bacting reuses code from Bioclipse, and in the period between the Bacting code being available and the Bacting paper being published, I like the credit to go to the closest thing. (In some future, Software Citations themselves will be a thing, but we're not there yet.)
Most of the other points are addressed in the various updates linked to this issue.
The compile issue I still open, tho. I have not been able to reproduce the problem. The error message did show JavaDoc warning and I fixed those.