322 Notebook Executables #329
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
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.
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).
- 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.
Currently non-functional, but a push for code review
12 tasks
daomcgill
reviewed
Nov 12, 2024
There was a problem hiding this comment.
I left my feedback, though it may be best if @carlosparadis puts eyes on this before you make the changes.
The flow of this script makes it difficult to read. I think enough so that you would want to restructure this. Take a look at the other exec scripts. The logic should look something like this:
1. if "build" and "help":
- useful message to describe build
2. else if "build":
- load config file from args
- use getters to set params
- call the function
- write output to file
3. if "parse" and "help":
- useful message to describe build
4. else if "parse" and file:
- load config file from args
- use getters to set params
- call the function
- write output to file
5. same thing with parse and class
See inline for my other comments. Notably: do not check for dir exists in here as it will be error-prone. Mark is working on making sure the directory will exist.
Oh, also do not forget to merge the necessary functions for testing/running this into the branch.
exec/understand.R
Outdated
| --file parses file-level dependencies | ||
| " | ||
|
|
||
| arguments <- docopt::docopt(doc, version = 'Kaiaulu 0.0.0.9600') |
exec/understand.R
Outdated
| understand_folder <- "" | ||
| code_language <- "" | ||
|
|
||
| if (arguments[["build"]]) { |
There was a problem hiding this comment.
in this if block, load in the config file that you passed in as a parameter e.g. conf <- yaml::read_yaml(conf_path)
|
Will try to take a look. I was going to say to start by merging master into this PR and we go from there! |
From my understanding of the feedback received, updated the understand.R.
Simple push after viewing during 12/5/2024 meeting
carlosparadis
requested changes
Dec 9, 2024
Standardizing understand.R as well as uploading depends and linemetrics executables. Additional testing required on non-Windows devices needed.
daomcgill
suggested changes
Dec 11, 2024
daomcgill
suggested changes
Dec 11, 2024
-Updated Functionality to encapsulate the third function "export" and the new parameter - Removed "description" tag from function
As directed by PR code review
…sailuh/kaiaulu into 322-creating-exec-for-notebooks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR for all execs to be made from notebooks