323 pulling request code review comment via GitHub rest api #330
Conversation
- Added `create_file_directory` function - Added `create_file_path` helper function
- Added @export to functions - Added some more detail to function description - Added discussion filepath under github issue tracker
- Updated `vignetts/download_github_comments.Rmd` - Added pull request comments to parse_github_replies function to `R/github.R` - Added new getter function to `R/config.R` - Updated configuration files with new file paths to save data from new endpoint
| @@ -12,10 +12,19 @@ vignette: > | |||
|
|
|||
| # Introduction | |||
|
|
|||
|
|
|||
| In this Vignette, we show how to download GitHub comments from both Issue and Pull Requests. This data may be useful if development communication occurs through issues or pull requests in GitHub in addition or instead of mailing lists. For details on how to merge various communication sources, see the `reply_communication_showcase.Rmd` notebook. The parsed communication table from GitHub comments by `parse_github_comments` has the same format as the mailing list and JIRA comments tables. In turn, the function expects as input the table generated at the end of this notebook. | |||
There was a problem hiding this comment.
"This data may be useful if development communication occurs through issues or pull requests in GitHub in addition or instead of mailing lists"
is not a well-worded or clear sentence. If I am understanding it correctly, maybe something along the lines of:
Downloading comments may be useful if communication during development occurs through issue or pull requests in addition, or in lieu of, mailing lists.
Clarifying ideas you're attempting to convey, especially with all of these nouns in one sentence, by elaborating terms like "development communication" to something understandable, rewording "addition or instead of" helps, or even what "data" is (in this case comments). This applies to all explanations, pretend as if the user has little-to-no knowledge of the functionality and terminology. In this case, throughout the notebook, possibly think of changing "development communication" to the self-explanatory "communication during development", or just explicitly explain it. The reason I say this is because my tired brain read "development communication" as "communication that's developing", and we need to find less ways to confuse our users.
Also, please show us or put a sample block of the expected parsed communication table or the expected input table- we want to reduce the amount of notebook jumping as much as possible
| @@ -125,7 +131,6 @@ all_issue_files <- lapply(list.files(save_path_issue_refresh, full.names = TRUE) | |||
| # Parse each JSON file using the refresh parser | |||
| all_issue <- lapply(all_issue_files, github_parse_search_issues_refresh) | |||
|
|
|||
|
|
|||
| # Combine all the data tables | |||
| all_issues_combined <- rbindlist(all_issue, fill = TRUE) | |||
|
|
|||
There was a problem hiding this comment.
For "Issues by Customized Queries", can you explain what parameters they can change? Why or when they would use such a function?
| @@ -218,9 +222,9 @@ head(all_issues_combined) %>% | |||
|
|
|||
| The Refresh parser also parses issue data but the folder 'refresh_issues', parsing data retrieved from the search endpoint. In this notebook, that is data from the REFRESH issues portion saved to issue_search directory. | |||
There was a problem hiding this comment.
What is that first sentence trying to convey? The two parts seemingly don't correlate?
- The Refresh parser also parses issue data but the folder 'refresh_issues'
- Parsing data retrieved from the search endpoint
Neither of these make sense alone, nor together. Unless the second is describing 'refresh_issues', it still does not make sense EVEN if we ignore it.
Also, do not meta-refer to the notebook like in "REFRESH issues portion". Find some way to say that the data retrieved from the ran functions is saved to issue_search directory.
|
You should also tell users if they which type(s) of GitHub tokens they can use, as well as the permissions they need to run the notebook- it should be explicitly explained so the users do not need do much other than consult the notebook. |
There was a problem hiding this comment.
@crepesAlot I was able to run through your notebook. I made some comments, but overall it looks good. I was able to understand what the notebook is about, and how to use it. The biggest issue I ran into was the github token file in the config part.
|
@daomcgill can you mark "request changes" as part of your review process here? |
|
@RavenMarQ same for you Raven, just so it is easier to know the status of your revisions. Appreciate if you could ask the others to do the same on the other PRs so it is more clear you reviewed each other's PR |
|
@carlosparadis should I repost the review to do this? |
|
@daomcgill No need to duplicate the comments, I would just click on the green review button for the pop-up and mark request for changes with a note "see comments posted in line for the PR Discussions". Please share with the group to do the same, it will be very confusing if the comments are re-added. Thank you for asking! |
|
*In the PR Conversation tab, not discussion sorry |
|
@daomcgill this is perfect Dao, thank you. The review requests gets hyperlink towards the end so that makes easier to find what is pending. I should have asked everyone to do the reviews as a group instead of comment so they were all grouped together but it is still fine like this. Once the changes are done, and you and Raven agree, you can both change to Accept. @RavenMarQ please do the same as Dao did with the request for changes and whichever other PRs you reviewed. Thank you! |
| @@ -12,10 +12,19 @@ vignette: > | |||
|
|
|||
| # Introduction | |||
There was a problem hiding this comment.
see comments posted in line for the PR Discussions
carlosparadis
changed the title
- Updated notebook to include section on where to place created tokens to use endpoints. - Updated pr_comments function documentation
- Made refresh function to use function parameter for the save_folder_path instead of a pre-assigned variable. - Updated various verbose print statements
- Retrieves `pull_request_review_id` from JSON file to refer to the review comment made when creating the review.
|
@daomcgill You think the section for .ssh folder was insufficient? What do you think should be added to the explanation? |
|
Wait, sorry. I did not see a section at all so that is my fault. Could you link the lines of code? |
|
@crepesAlot I see it now. Yeah, I think that is sufficient. My bad! |
Using the REST API to download review comments from Pull Requests using the
GET /repos/{owner}/{repo}/pulls/commentsendpoint.