Scitools Understand Parser #309
Conversation
Modeled after parse_dependencies with a similar output, this is for review to see if it is in-line with specification
Per the specifications in Issue 308, the files are functional and ready to be put into a notebook.
Quick fix
Updating NAMESPACE to export the new functions and creating the Rmd, are the primary notes. The folder holding the sample project also is included locally, but uses the calculator project provided in Issue from Carlos
|
@RavenMarQ just a reminder the notebook should have more than just code, please check with @daomcgill or ask her for a code review if needed. |
There was a problem hiding this comment.
I am a bi concerned with the use of loops here and the overall code logic, did you follow
https://github.com/sailuh/kaiaulu/blob/c7811069ef3c7943b509c76d4b9116d1a14dc343/R/src.R#L373C1-L378
?
|
In addition to the above, see CONTRIBUTING.md on how to edit NEWS.md, and DESCRIPTION. A lot of files that shouldn't be added have been pushed in this PR and those that should be edited were not. |
Most of the code review notes have been resolved, except for changing the file.path and descriptions. The notebook is currently being updated, but a preview on the proposed format is provided.
There was a problem hiding this comment.
The reason for having a notebook is to help users understand what they can do with it, and why they would want to use the features in the first place (as well as understanding how to use the code). The user should not need to read through your issue or code in order to know what is going on. This is the place to explain it to them.
Having addressing most things, especially using file.path, the functions are functional and the notebook works.
Resolving comments received on the notebook.
- Refactored the understand_showcase.Rmd notebook to expect the use of the getters from R/config.R (i #230 contains the getter functions in R/config.R).
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #309 +/- ##
==========================================
- Coverage 39.79% 37.94% -1.85%
==========================================
Files 20 20
Lines 3091 3355 +264
==========================================
+ Hits 1230 1273 +43
- Misses 1861 2082 +221 ☔ View full report in Codecov by Sentry. |
- The project configuration sections of a notebook was incorrectly using the project directory (kaiaulu/) as its working directory rather than the directory that it resides in (/vignettes/) as its working directory.
|
@daomcgill the GitHub Actions problem was incidentally isolated: The moment I merged The GitHub Refresher, master started passing. I thought GitHub Actions broke something, but the problem is deeper and in our end. Notice how @RavenMarQ PR also passed just now after I merged the passing master. @beydlern @crepesAlot please merge master to your PRs that are still open. master is currently passing, as so is this PR. If yours is not passing, I can't merge. Ping me on your PRs that do not pass master so we can diagnose. If you had to merge Dao's PR, then the issue on her code likely spread to yours. |
This is to test Committing and PR