Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mail count & Issue count #194

Merged
merged 21 commits into from
Apr 30, 2021
Merged

Mail count & Issue count #194

merged 21 commits into from
Apr 30, 2021

Conversation

JoJoDeveloping
Copy link
Contributor

@JoJoDeveloping JoJoDeveloping commented Jan 27, 2021

Prerequisites

  • I adhere to the coding conventions (described here) in my code.
  • I have updated the copyright headers of the files I have modified.
  • I have written appropriate commit messages, i.e., I have recorded the goal, the need, the needed changes, and the location of my code modifications for each commit. This includes also, e.g., referencing to relevant issues.
  • I have put signed-off tags in all commits.
  • I have updated the changelog file NEWS.md appropriately.
  • The pull request is opened against the branch dev.
  • All tests pass

Description

Adds methods that add relevant attributes to implement the changes noted in #188
These are per-author and include:

  • issue count
  • issue comment count
  • mail count
  • mail thread count
  • PR count
  • PR comment count

The definition of when an author is related to an issue are tentative and should be looked over. In particular we have:

  • An issue is counted for someone if they participated in that issue in any way (i.e. labeling, commenting, opening/closing, mentioning)
  • An issue is counted for someone if they comment in it
  • A PR is counted for the person opening it
  • A PR is counted for a person when they comment in it (this includes the initial author, mostly, as they write the initial description)
  • A PR is counted for a person when they participate it in any way (i.e. labeling, commenting, opening/closing, mentioning, adding comments to it)

As an added bonus, we also fix #185. We also rename the issue data getters in util-data.R so that they are named consistently with the commit data getters.

Merging this closes #185, closes #188.

@JoJoDeveloping JoJoDeveloping changed the base branch from master to dev January 27, 2021 11:51
@JoJoDeveloping JoJoDeveloping force-pushed the mail-count branch 4 times, most recently from e494112 to a0f9992 Compare January 27, 2021 13:08
@JoJoDeveloping
Copy link
Contributor Author

I don't know why CI fails. I can build it locally and run the tests just fine. The CI also only fails for version 3.3 with an error indicating sqldf is missing (?).

Copy link
Collaborator

@bockthom bockthom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @JoJoDeveloping for this very nice pull request! 🥳 I really appreciate it. In particular, I like your abstractions and, especially, also the different kinds of how to count issues and pull requests. 👍 Nevertheless, as this affects also filtering and configuration options of the project data, we should discuss about that together in our next meeting, as this also may affect code beyond this pull request. For that reason, I did not yet have a look at your changes in util-networks-covariates and also not at your tests. Stay tuned!

In the meanwhile, there are a few minor issues you could work on 😉 For example, your fancy helper function would be even fancier if you would add a few inline comments. For more details, see my comments below.
When reading my comments, please also keep in mind that some of the coding-style related comments also hold for several other places in your implementation than just the one I have added the comment at.

In case of questions, please don't hesitate to ask.

util-core-peripheral.R Outdated Show resolved Hide resolved
util-core-peripheral.R Outdated Show resolved Hide resolved
util-core-peripheral.R Outdated Show resolved Hide resolved
util-core-peripheral.R Outdated Show resolved Hide resolved
util-core-peripheral.R Outdated Show resolved Hide resolved
util-core-peripheral.R Outdated Show resolved Hide resolved
util-core-peripheral.R Outdated Show resolved Hide resolved
util-core-peripheral.R Outdated Show resolved Hide resolved
util-core-peripheral.R Outdated Show resolved Hide resolved
util-data.R Outdated Show resolved Hide resolved
Copy link
Contributor

@hechtlC hechtlC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @JoJoDeveloping . I found a couple of additional things which were not mentioned in @bockthom's review.

tests/test-networks-covariates.R Outdated Show resolved Hide resolved
tests/test-networks-covariates.R Outdated Show resolved Hide resolved
util-core-peripheral.R Outdated Show resolved Hide resolved
util-core-peripheral.R Outdated Show resolved Hide resolved
util-core-peripheral.R Outdated Show resolved Hide resolved
util-core-peripheral.R Outdated Show resolved Hide resolved
Copy link
Collaborator

@clhunsen clhunsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your PR, @JoJoDeveloping! 😃 Keep up the good work!

This is not a full review as I have not looked into the implementation in detail. I just want to share my thoughts on some things that I spotted while scanning the comments of @bockthom and @hechtlC. 😉

util-core-peripheral.R Outdated Show resolved Hide resolved
util-core-peripheral.R Outdated Show resolved Hide resolved
util-core-peripheral.R Outdated Show resolved Hide resolved
util-core-peripheral.R Outdated Show resolved Hide resolved
util-data.R Outdated Show resolved Hide resolved
Copy link
Contributor

@hechtlC hechtlC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all thanks for the good work on this enhancement @JoJoDeveloping.

There are some problems here (mostly documentation). These will need to be fixed. Please also check whether there are more than one occurrence of the documentation problems as I have not marked every one.

Please also don't forget to address the old comments.

NEWS.md Outdated Show resolved Hide resolved
tests/test-networks-bipartite.R Show resolved Hide resolved
tests/test-networks-covariates.R Outdated Show resolved Hide resolved
tests/test-networks-covariates.R Outdated Show resolved Hide resolved
tests/test-networks-covariates.R Outdated Show resolved Hide resolved
util-data.R Outdated Show resolved Hide resolved
util-networks-covariates.R Outdated Show resolved Hide resolved
util-networks-covariates.R Outdated Show resolved Hide resolved
util-networks-covariates.R Outdated Show resolved Hide resolved
util-split.R Outdated Show resolved Hide resolved
Copy link
Collaborator

@bockthom bockthom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @JoJoDeveloping for working on this PR. I just had a quick view at it and spotted only minor issues. I will do a full pass when all the remaining issues (especially to the test suite) are fixed.

I have already spotted a few issues:

  1. This PR contains files which should not be part of it, e.g., tests/testthat-problems.rds. Please remove them via rebase, as we don't want such files to show up in the commit history. Please also check if there are other files that should not appear here.
  2. There still are some violations of the coding conventions (spacing, indentation, etc.). The individual comments below just indicate some of them. Please also check for similar violations not explicitly mentioned in our reviews.
  3. The test data for pull-requests contains unrealistic data. Please adjust events in such a way that they can occur in a real setting, see also my comments below.

Please see also the individual comments below.

NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
tests/test-data.R Show resolved Hide resolved
util-core-peripheral.R Outdated Show resolved Hide resolved
util-core-peripheral.R Outdated Show resolved Hide resolved
util-core-peripheral.R Outdated Show resolved Hide resolved
util-core-peripheral.R Outdated Show resolved Hide resolved
util-read.R Outdated Show resolved Hide resolved
@bockthom bockthom added this to the v3.8 milestone Mar 18, 2021
@bockthom bockthom mentioned this pull request Mar 19, 2021
6 tasks
Tests for PRs are missing since there is no test data including PRs yet

Signed-off-by: Johannes Hostert <[email protected]>
…n all varieties one can imagine (and more)

Signed-off-by: Johannes Hostert <[email protected]>
…t.issues to make it similar to get.commits[.filtered]

Close se-sic#185.

Signed-off-by: Johannes Hostert <[email protected]>
Signed-off-by: Johannes Hostert <[email protected]>
Copy link
Collaborator

@bockthom bockthom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yet another review on my part 😃

In the end, I have added way more comments than I wanted to add... but I think that we are close to merging, though, if the comments are adressed. However, the longer the time span between two subsequent reviews, the higher the probability that I will spot new issues that I have not spotted yet 😉
Maybe you would like make use of this information when addressing my comments 😄
Anyway, please find my detailed comments below. If anything is unclear, don't hesitate to ask.

util-data-misc.R Outdated Show resolved Hide resolved
util-data-misc.R Outdated Show resolved Hide resolved
util-data-misc.R Outdated Show resolved Hide resolved
util-data-misc.R Outdated Show resolved Hide resolved
util-data-misc.R Outdated Show resolved Hide resolved
util-read.R Outdated Show resolved Hide resolved
tests/test-networks-covariates.R Outdated Show resolved Hide resolved
tests/test-networks-covariates.R Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
util-data-misc.R Outdated Show resolved Hide resolved
util-data-misc.R Outdated Show resolved Hide resolved
util-data.R Outdated Show resolved Hide resolved
showcase.R Outdated Show resolved Hide resolved
@JoJoDeveloping JoJoDeveloping force-pushed the mail-count branch 2 times, most recently from 2de0157 to d9a9366 Compare April 29, 2021 08:20
showcase.R Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
@bockthom
Copy link
Collaborator

bockthom commented Apr 29, 2021

I'm done with my final review! As soon as the stated issues regarding test, news, and showcase/util-networks-metrics are fixed, this PR can be merged.

@bockthom
Copy link
Collaborator

bockthom commented Apr 29, 2021

Thanks @JoJoDeveloping for your endurance. This PR contains a lot of substantial and valuable changes and will definitively improve coronet in various directions.
All the implementation-related issues (at least, those which we have spotted) are fixed now, looks really good 😄 👍

The only point of criticism left is that the last commit's message does not contain a signed-off tag 😉

@bockthom bockthom changed the title Mail count Mail count & Issue count Apr 30, 2021
@bockthom bockthom merged commit 9a4ccb1 into se-sic:dev Apr 30, 2021
bockthom added a commit to bockthom/coronet that referenced this pull request Jul 19, 2021
When not passing a specific value for parameter `issue.type` to all the
`add.vertex.attribute.issue.*` functions, the vector `c("all", "pull.requests",
"issues")` is automatically used as default. However, as only one of the
vector's elements is allowed, this leads to problems when passing this parameter
to other functions in turn. To fix this, add some missing `match.arg` calls that
make sure that only one value (the actual default "all") is used for the
`issue.type` parameter.

This is a follow-up commit for PR se-sic#194 and commit
eb4f649.

Signed-off-by: Thomas Bock <[email protected]>
@bockthom bockthom mentioned this pull request Sep 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants