egonw
commented
4 years ago 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.
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:
[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).