-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Code quality issues #50
Comments
Any updates on this yet? |
Hi, sorry for the delay - I'm on parental leave for another two weeks but making some progress with collaborators in some free moments. I will plan to update again in around three weeks or less, if that sounds okay. |
@fabian-s do you know of any clever way to identify the functions imported from other packages? I've tried pkgnet and lintr to no avail. Of course, a manual approach may be best - planning to do that. I think we've addressed the other issues; will detail how and in which commits shortly. |
sounds dumb but should work:
alternatively, run |
For some reason, after removing imports and suggests, rebuilding, and checking, I am only seeing:
When there are far more. |
yeah, sorry about that, -- need to delete the NAMESPACE directives as well. if i do so and run
I see:
proceed from there, I guess |
the functions you import via "::" or ":::" are:
|
@fabian-s Thanks to your suggestion, we roxygen2 commented the dependencies of each function, regenerated the NAMESPACE file, and updated the Description file, and in the end it basically passed the R CMD check!
── R CMD check ───────────────────────────────────────────────────────────────────────────────────────────────── See ── R CMD check results ───────────────────────────────────────────────────────────────────── konfound 0.4.0 ──── ❯ checking for hidden files and directories ... NOTE 0 errors ✔ | 0 warnings ✔ | 1 note ✖ |
not the case for me on commit 78ec6f9, I see
which is unacceptable for R packages looking to be published on JOSS. Additionally, none of the issues found by goodpractice::gp and pointed out by me in August seem to have been adressed sucessfully:
At JOSS, we aim for a collaborative, constructive review style. |
EDIT: maybe you did not push those latest changes to the repo --- the commit I'm on is: commit 78ec6f9 (HEAD -> master, origin/master, origin/HEAD) |
@fabian-s Yes, the team hasn't merged some of the changes on the default master branch yet, we'll agree on that as soon as possible. |
ok please don't @ me again before that is done and you have an actually working and standards-compliant package, ty |
Sorry all, this is my fault - needed to manage the multiple branches better. We're meeting to address. |
What actually happened here is you somewhat successfully refactored test_sensitivity_ln (217 -> 59), while cyclocomp values for every other function remain completely unchanged since I opened this issue (e.g. git blame for getswitch_fisher tells me there have not been any structural changes at all in the last 4 years). Most of this seems to be covered by tests now, so at least you may notice once your convoluted stuff breaks.
EDIT: at least these remain open:
|
We will address those issues. |
Also, I want to register that I was not attempting to mislead you! I wrote: "We have made changes to the code to reduce the cyclometric complexity, ...". I did not (and did not mean to!) suggest that we addressed the other issues or that this broader issue was resolved -- I was commenting on the cyclometric complexity aspect of these issues related to the code quality. |
I recognize this has been a very rocky submission. Unusually, I want to share this to rebuild some good faith. Here's a conversation with @wwang93 and I discussing how while the cyclometric complexity was partially addressed, there were lingering issues that Wei was planning to address. |
#78 addresses:
We have NOT yet addressed this:
It is not immediately clear to us what is causing that not yet addressed issue, as we have used |
The
|
We have addressed the remaining goodpractice issue; as an update:
From
|
Seems fine, thanks |
goodpractice::gp finds very low test coverage and some inconsistent / non-compliant coding style.
The former at least should be improved for your JOSS publication
cyclomatic complexity of many of your functions is extremely high, casting doubt on maintainability, legibility and correctness of the implementation.
maybe try refactoring/getting rid of some of your conditional logic and/or modularize your functions to get cyclomatic complexities closer to reasonable values of like 20-50?
crossref: openjournals/joss-reviews#5779
The text was updated successfully, but these errors were encountered: