Closed Rubegen closed 1 year ago
Merging #233 (42d1f80) into master (01142c5) will increase coverage by
3.12%
. The diff coverage is100.00%
.:exclamation: Current head 42d1f80 differs from pull request most recent head 0fb7ef2. Consider uploading reports for the commit 0fb7ef2 to get more accurate results
@@ Coverage Diff @@
## master #233 +/- ##
==========================================
+ Coverage 14.31% 17.44% +3.12%
==========================================
Files 19 20 +1
Lines 2445 2534 +89
==========================================
+ Hits 350 442 +92
+ Misses 2095 2092 -3
Files | Coverage Δ | |
---|---|---|
R/example.R | 100.00% <100.00%> (ø) |
|
R/git.R | 80.76% <100.00%> (+8.24%) |
:arrow_up: |
R/io.R | 83.33% <100.00%> (+83.33%) |
:arrow_up: |
Small side note:
Please do not delete the fork as you try to change the code! This will kill the Pull Request, and we don't want to create one PR to every attempt!
@Rubegen
The last request for changes going by my comment was 4 weeks ago and your last commit 2 weeks ago. Could you give me an update here?
Thanks!
Hi Carlos, I've implemented the parameter and function changes that we discussed at the meetings, and I have been trying to install perceval on my machine to ensure the PR tests all pass. I was able to install perceval on my machine using pip3 but the tests still fail when I run them on RStudio. The 'Which Perceval' command also does not return anything and I am figuring out why. I wanted to commit my changes to keep you updated on my code but I am still working out the perceval problems on my end.
@Rubegen Did you try to search your computer for the file "Perceval" to see if it finds anything? I believe I misremembered which
perceval to return, but searching the computer should suffice to find the file.
I found the file in my python packages. Namely /Users/rubenjacobo/Library/Python/3.8/lib/python/site-packages but I still am not able to call it. Using 'perceval' on terminal always returns 'command not found' but I am sure that I have it installed. Should I still copy and paste that directory that leads to perceval into the tools.yml page?
Can't you just run the command in the folder it is located? You don't need to be able to type "perceval" to run the tool, just have its path added on tools.yml. If it runs on its own folder it is fine.
Note you are modifying the DESCRIPTION file and the tools.yml. Neither file should be being modified by this PR. The change of version in the DESCRIPTION file may be the reason you are not passing the checks, as it is leading for the XML package to not be found. You need to reverse the change and git squash the PR to address this.
Updating your fork with most recent Kaiaulu changes, let's see if that fix your checks.
Your checks are now passing, and I tried to help you on the more complicated parts of the code. However, there is still work to be done on the code documentation, function naming, etc.
You also have to carefully test the code. If you ran the git_delete_sample_log for example, you would be able to see the folder was not being deleted.
all supressWarnings should be removed and the warnings addressed.
There are functions with missing title and incomplete descriptions.
Finally, please coordinate with @waylonho so your git.R
are the exact same file since you both depend on the git.R file. The examples.R should only include your own functions, not the other. They should be merged after I accept either of yours into master. The same for unit tests: Only yours should be present on your commit.
Please spend some time studying the changes and trying them out yourself and comparing to your current version to see why yours did not work and these now pass the tests. I recommend you git clone this branch of the repo in a separate folder on your computer so you can compare side by side.
Thanks.
Hi Carlos, thank you for the code additions. I am currently coordinating with Waylon to ensure we're on the same page for the functions that are needed for both of our Unit tests to pass. I looked through your comments on the code and I am working on making the proper adjustments for the prefix and suffix tests. I aim to have a new commit done by tomorrow. Just wanted to keep you posted on my work. Thank you
Thank you for staying in touch! Let me know if anything is not clear. Happy to clarify.
Forgot to mention: you both also modified and need the io.R
file, so please also do make them consistent between both PRs (like the git.R). @waylonho notice I added a new function to delete folders on @Rubegen PR. You should use this over the git_delete_sample function where needed.
@Rubegen @waylonho
Well done! Most of what remained was "cosmetics", but nonetheless important. Please check commit 0fb7ef2 for the subsequent milestone, but also for PR #232 commit. Also note NEWS.md was missing on this commit.
@waylonho Note I made minor renaming on the git.R
and io.R
functions, etc so please adjust your example.R
and test-parser.R
to reflect these. Please also consider the fixes I made such as the docs, order of parameters of the example, etc.
@Rubegen with this merge, you're officially a contributor :) Your name should be appearing on the front page of the repo too. Once I update the docs on itm0, it should start showing up there too under the Other Authors page.
@waylonho Note in the merge commit I also noted you as co-author since some of git.R and io.R functions were yours.
Thanks!
…e_sample_file, git_init