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

Vertex attributes for artifacts #229

Merged
merged 23 commits into from
Oct 24, 2022
Merged

Conversation

joba00002
Copy link
Contributor

@joba00002 joba00002 commented Aug 5, 2022

Addresses #170

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.
  • I have checked whether I need to adjust the showcase file showcase.R with respect to my changes.
  • The pull request is opened against the branch dev.

Description

Changelog

@joba00002 joba00002 force-pushed the artifact_attributes branch 3 times, most recently from c93428d to 2922050 Compare August 10, 2022 10:58
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 for your additions to this PR @joba00002. Sorry for the late reply, we were very busy during the last weeks.
I had a quick look at your recent changes and spotted a few possible copy-paste errors or statements that need more documentation, etc., please see my individual comments below.

In addition, I checked the list of vertex attributes in #170. Could you please tick all the attributes you already have implemented? From what I have seen now, only two vertex attributes are still missing, right?

  • number of comments in an issue (there is an attribute for events already, but maybe someone would like to count the comments only even if all events are configured to be considered for network construction)
  • originating mailing list

Please let us know if you have further questions regarding that.

@joba00002
Copy link
Contributor Author

I wanted to tick the ones I have implemented, however, GitHub doesn't seem to allow me to do so. I might not have the necessary permissions to edit that issue.

Regarding the originating mailing lists of mail threads, I thought you said in a meeting that the thread ID contains the index of the mailing list as listed in the config file.
However, at least in the sample data, this does not appear to be the case, so I currently don't see a way of linking a thread to its originating mailing list.

@joba00002 joba00002 force-pushed the artifact_attributes branch 2 times, most recently from 407b18a to 28f3732 Compare September 13, 2022 15:30
Add metrics for issues and mail threads.

Signed-off-by: Jonathan Baumann <[email protected]>
Includes start and end dates, contributor count and activity count.

Signed-off-by: Jonathan Baumann <[email protected]>
Added myself to util-networks-covariates.R and util-data-misc.R

Signed-off-by: Jonathan Baumann <[email protected]>
Inserted line breaks for lines that were longer than 120 characters.

Signed-off-by: Jonathan Baumann <[email protected]>
Also adjust default names and default values.

Signed-off-by: Jonathan Baumann <[email protected]>
Includes date an artifact was added, issue titles, and whether an issue is a pull request.

Signed-off-by: Jonathan Baumann <[email protected]>
Fix a broken function, avoid code duplication

Signed-off-by: Jonathan Baumann <[email protected]>
Only counts the number of comments, not other issue events.

Signed-off-by: Jonathan Baumann <[email protected]>
Also includes artifact last edited date

Signed-off-by: Jonathan Baumann <[email protected]>
For issues only, PRs only, and both

Signed-off-by: Jonathan Baumann <[email protected]>
Also includes some tests.

Signed-off-by: Jonathan Baumann <[email protected]>
@joba00002 joba00002 force-pushed the artifact_attributes branch 2 times, most recently from 6a1bb14 to 2fb72e7 Compare September 27, 2022 14:04
Old names were confusing, at least this is a consistent pattern

Signed-off-by: Jonathan Baumann <[email protected]>
issue.event.count, issue.comment.event.count, and issue.is.pull.request

Signed-off-by: Jonathan Baumann <[email protected]>
issue.opened.date, issue.closed.date, issue.last.activity.date,
issue.title

Signed-off-by: Jonathan Baumann <[email protected]>
As the `issue.state` only displays "open" or "closed", this vertex
attribute can additionally determine whether a PR was closed by merging
or rejected.

Signed-off-by: Jonathan Baumann <[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.

Today, I've gotten round to review this PR. Thanks for your excellent work on this PR @joba00002!

I could not yet have a look at all of the tests as checking the expected data is very exhausting. I aim at checking the expected results on a sample basis later this week.

Except for the tests, I did a detailed review and spotted a few minor issues that need to be fixed. Please have a closer look at my individual comments. If anything is unclear, please don't hesitate to ask.

incorrect log messages, more comments, reopened merge requests now count
as open, and events with multiple lines now only counted once.

Signed-off-by: Jonathan Baumann <[email protected]>
While this is not necessary now, it's possible that filtering options
may change in the future, making this useful.

Signed-off-by: Jonathan Baumann <[email protected]>
Codeface produces thread IDs prefixed with the mailing list ID. Test
data now reflects this.

Signed-off-by: Jonathan Baumann <[email protected]>
for each mailing list vertex, adds an attribute with the identifier of
the corresponding mailing list.
Currently, this is just a number, as no further information is provided
by codeface.

Signed-off-by: Jonathan Baumann <[email protected]>
In the currently used R containers in our CI pipeline, the installation
of R package 'systemfonts' (which is a dependency of package 'ggraph')
fails as the system library 'libfontconfig1-dev' is not installed within
these containers. Hence, install 'libfontconfig1-dev' within the
pipeline before executing the test scripts.

Signed-off-by: Thomas Bock <[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.

Thanks @joba00002 for all the efforts, looks good to me!

I just spotted a couple of spacing inconsistencies. Please fix them and add your changes to the NEWS.md, and then this PR is ready to merge.

Spaces around +, newlines before '@return'

Signed-off-by: Jonathan Baumann <[email protected]>
@joba00002 joba00002 force-pushed the artifact_attributes branch from 98179aa to 3a4b794 Compare October 22, 2022 10:29
Signed-off-by: Jonathan Baumann <[email protected]>
@joba00002 joba00002 force-pushed the artifact_attributes branch from 3a4b794 to aac337c Compare October 22, 2022 10:31
@bockthom bockthom added this to the v4.2 milestone Oct 24, 2022
@bockthom bockthom merged commit 68c0515 into se-sic:dev Oct 24, 2022
@bockthom bockthom mentioned this pull request Oct 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants