Closed leilani-reich closed 1 year ago
Haha, indeed 31 files. Could you let me know which files you want me to review? R/download.R for certain, but what were the others? I can work with that.
Yes, it is just the R/downloader functions download_bugzilla_issues_from_rest_api() and download_bugzilla_comments_from_rest_api(). And the NEWS.md, _pkgdown.yml, CONTRIBUTING.md, and the NAMESPACE, and the man files for the two downloader functions.
Actually, I think I added the bugzilla_wrapper_showcase notebook by accident. I will double-check what I added and redo the commit with the notebook removed if that's ok.
EDIT: Yes the files above are what I changed. Should I remove the bugzilla_wrapper_showcase notebook right now or after you review my changes?
Yes, that's fine. Go ahead and make the needed changes and post another comment when ready to go :)
Hi Carlos, I removed the file that shouldn't have been in my commit. Thanks!
Reply to https://github.com/sailuh/kaiaulu/pull/177#pullrequestreview-1378369733
Here's what the output looks like for my download_bugzilla_issues_from_rest_api function
And for my download_bugzilla_comments_from_rest_api function
Note: I made the folders to hold the issues/comments manually.
Reply to https://github.com/sailuh/kaiaulu/pull/177#issuecomment-1502714303
Ok! Looks good :) Just to be clear, the file names for the issues are the pages, which contain a group of topics, and for the comments, each file is a comment, and the file name is a comment id, correct?
Also, after you make the final changes, could you try testing the final version against just another bugzilla project to see if it works too? That way we are sure we didn't hardcode anything too specific for Red Hat.
Note to self so I don't forget:
This is pending so we can accept the PR: https://github.com/sailuh/kaiaulu/pull/177#pullrequestreview-1378369733
Reply to https://github.com/sailuh/kaiaulu/pull/177#issuecomment-1502779845
Reply to #177 (comment)
Ok! Looks good :) Just to be clear, the file names for the issues are the pages, which contain a group of topics, and for the comments, each file is a comment, and the file name is a comment id, correct?
The filenames for the issues are the pages. Each page contains up to 20 different issues. However, for the comments folder, each file is a group of comments for a specific issue. I name each file in the comments folder by the issue id the group of comments in each json is associated with.
Also, after you make the final changes, could you try testing the final version against just another bugzilla project to see if it works too? That way we are sure we didn't hardcode anything too specific for Red Hat.
Yes, I have also tested this with https://bugs.webkit.org. I will try with a few more bugzilla sites.
Reply to https://github.com/sailuh/kaiaulu/pull/177#issuecomment-1502829995
Got it. Could you include the answer to my questions as part of the documentation body (so right below the docs title, where the user can read it if they pull the function docs with ?) of the two functions too? I think this will be helpful for the users!
This PR has conflicting files on R/parser.R and man/parse_mbox.Rd according to GitHub. You may need to Git Pull and squash to address them once you are done with the revisions.
Merging #177 (02aad9b) into master (a3b4ffc) will decrease coverage by
0.06%
. The diff coverage is0.00%
.:exclamation: Current head 02aad9b differs from pull request most recent head 5edb538. Consider uploading reports for the commit 5edb538 to get more accurate results
@@ Coverage Diff @@
## master #177 +/- ##
=========================================
- Coverage 2.79% 2.74% -0.06%
=========================================
Files 15 15
Lines 1970 2007 +37
=========================================
Hits 55 55
- Misses 1915 1952 +37
Impacted Files | Coverage Δ | |
---|---|---|
R/download.R | 0.00% <0.00%> (ø) |
Oops...not ok I see conflicts. Let me try to resolve them myself.
Checks pass affect merge conflict is resolved. So good to go.
I have updated my downloader function according to feedback on https://github.com/sailuh/kaiaulu/issues/153#issuecomment-1482339183.
The changes added are mainly in R/downloader.R. I also updated NEWS.md, _pkgdown.yml, CONTRIBUTING.md, and the NAMESPACE.
I was trying to get rid of merge commits with a git squash, but the git squash doesn't show merge commits, so I did a git reset to make only a single commit for my bugzilla downloader. However, I think it shows a lot more unnecessary stuff now. Sorry about that. I can redo it if that's better.