Skip to content

Conversation

bockthom
Copy link
Collaborator

@bockthom bockthom commented Dec 10, 2024

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

While we already have functionality to compute the first activity per developer, there was no functionality yet to compute the last activity per developer. With this PR, functionality to compute the last activity per developer is added.

Changelog

Added

  • Add function to compute last activity per person and activity type
  • Add function to add vertex attribute "last.activity"

@bockthom bockthom added this to the v4.5 milestone Dec 10, 2024
@bockthom bockthom requested a review from maxloeffler December 10, 2024 18:35
@maxloeffler
Copy link
Contributor

@bockthom I have had a closer look at the changes submitted in this PR and I could not find major issues with it! I only spotted some minor stylistic ones, I will list them below.

@bockthom bockthom force-pushed the thomas-updates branch 2 times, most recently from f4e0214 to b2b32d8 Compare December 17, 2024 12:05
@maxloeffler
Copy link
Contributor

Thank you @bockthom for correcting the misalignments all over util-networks-covariates.R! I only spotted three tiny misalignments that slipped through (see below). Apart from those, I approve this PR.

@maxloeffler
Copy link
Contributor

The elements in the aggregation.level c(...) are not one space off:

add.vertex.attribute.author.role.function = function(list.of.networks, project.data, classification.function,
name = "author.role",
aggregation.level = c("range", "cumulative", "all.ranges",
"project.cumulative", "project.all.ranges",
"complete"),
default.value = NA) {

Same goes for the elements in the aggregation.level c(...) here:

add.vertex.attribute.author.issue.comment.count = function(list.of.networks, project.data,
name = "issue.comment.count",
aggregation.level = c("range", "cumulative", "all.ranges",
"project.cumulative",
"project.all.ranges", "complete"),
default.value = 0L,
issue.type = c("all", "pull.requests", "issues"),
use.unfiltered.data = FALSE) {

Lastly, these parameters are just overall off:

add.vertex.attribute.artifact.last.edited = function(list.of.networks, project.data, name = "last.edited",
aggregation.level = c("range", "cumulative", "all.ranges",
"project.cumulative",
"project.all.ranges", "complete"),
default.value = NA) {

Note: As you did not edited these lines, it would not let me create a regular review comment and I had to resort to this.

@bockthom
Copy link
Collaborator Author

bockthom commented Jan 12, 2025

I've fixed the three misalignments that you have spotted @maxloeffler (at least, I hope so). If all the misalignments you have spotted are fixed now and if you agree with my descriptions in the NEWS.md, I'd be happy if you could also formally approve this PR 😄

While a function to compute the first activity per person and activity type
already existed, this was not the case for the last activity.

Instead of copying the code from first-activity computation, reuse the code by
establishing an additional helper function for aggregating activity dates. This
is used by first and last activity now, and could potentially also be used for
other aggregations (e.g., to compute the mid date of a person's activities).

Signed-off-by: Thomas Bock <[email protected]>
Similar to enhancing first-activity computation by last-activity computation in
the previous commit, this can also be done for the corresponding vertex
attributes.

We now have a new function to attribute the "last.activity" vertex attribute.
Both functions to add first and last activity as a vertex attribute make use of
a commonly used helper function that allows to add an aggregated activity date
as a vertex attribute.

Signed-off-by: Thomas Bock <[email protected]>
The tests cover the new "last.activity" vertex attribute, which shall implicitly
also cover the last-activity data computation.

Signed-off-by: Thomas Bock <[email protected]>
Signed-off-by: Thomas Bock <[email protected]>
Some links to Codeface in our README.md still pointed to siemens/codeface,
which had been archived already two years ago. Instead of these outdated
links, we link to the se-sic/codeface repository now.

This partly addresses se-sic#272.

Signed-off-by: Thomas Bock <[email protected]>
Copy link

codecov bot commented Jan 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.13%. Comparing base (58f66cf) to head (3e40555).
Report is 7 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #275      +/-   ##
==========================================
+ Coverage   81.09%   81.13%   +0.03%     
==========================================
  Files          16       16              
  Lines        5095     5105      +10     
==========================================
+ Hits         4132     4142      +10     
  Misses        963      963              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bockthom
Copy link
Collaborator Author

As codecov is fine with the coverage of my tests, and as @maxloeffler has already approved the PR, I will merge right away.

@bockthom bockthom merged commit b018a43 into se-sic:dev Jan 14, 2025
9 checks passed
@bockthom bockthom mentioned this pull request Feb 2, 2025
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