Add possibility of editor definition to add.vertex.attribute.artifact.editor.count

[klaraschlueter]

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.

Description

Add a parameter 'editor.definition' to the function 'add.vertex.attribute.artifact.editor.count' which can be used to define, if author or committer or both count as editors when computing the attribute values.

Changelog

Add a parameter 'editor.definition' to the function 'add.vertex.attribute.artifact.editor.count' which can be used to define, if author or committer or both count as editors when computing the attribute values. (ff1e147)

[clhunsen]

Thanks for your PR, @klaraschlueter! And congrats on opening the first PR in the new coronet repository! 🎉

To increase code readability, I have two suggestions for your code. 🙂

[clhunsen]

Ah, another comment because I forgot to look at the changelog 🤦‍♂️: Can you please add the corresponding issue to the changelog entry? Also, please use backticks (`) for code there.

[bockthom]

I am not sure if c("author", "committer") is the default value we want to have here. I would go for just "author" as default value, as in most cases we are interested in the author of a commit and currently we only rely on the author in many other functions.

@clhunsen @klaraschlueter What do you think about that?

[clhunsen]

As the integration of committer data is not completely rolled out yet (see also #84), using "author" as default value would be reasonable... 🤔

I looked at the function match.arg.or.default, and it does not work properly for the current use case (due to restrictions on the parameter default). Consequently, we need some implementation work for this to run. For now, I would like to propose the following snippet to solve the current situation:

if (missing(editor.definition)) {
    editor.definition = "author"
} else {
    editor.definition = match.arg.or.default(editor.definition, default = "author", several.ok = TRUE)
}

[klaraschlueter]

Using author as default sounds good to me @bockthom , another reason is backwards compatibility.

For what kind of call does match.arg.or.default return unexpected results, @clhunsen ?

[bockthom]

match.arg.or.default does not return unexpected results (as they are properly documented), but ignores the default parameter if several.ok = TRUE and returns the complete vector of allowed values.

[klaraschlueter]

Ah, I see. I ran some tests with your snippet, @clhunsen , (with one small change: I added the parameter choices = c("author", "committer") to the call to match.arg.or.default, since we decided for "author" as default).
I think, we do not need your if-case: if no editor.definition is given, the default value is inserted during the call to add.vertex.attribute.artifact.editor.count and match.arg.or.default returns the expected result "author". A problem seems to arise if I call the function with a nonsense editor definition, for example: editor.definition = nonsense: The call to add.vertex.attribute.artifact.editor.count and the if-case are "satisfied" since there is something and do not insert the default value, and match.arg.or.default throws an error: 'arg' should be one of “author”, “committer”.

[klaraschlueter]

Okay, an update for you, @clhunsen : @bockthom and I agreed that a call with a nonsense editor definition should lead to an error, so everything works as expected if I call match.arg.or.default and specify the parameter `choices = c("author", "committer").

[klaraschlueter]

@clhunsen , @bockthom : done, committed, pushed!

[bockthom]

Thanks for this PR, @klaraschlueter.

As already mentioned in a personal meeting, could you please cherry-pick the following commit from my fork and add it to this pull request: 3a203cb

Thanks a lot in advance.

[clhunsen]

Thanks for the changes, @klaraschlueter! I will merge in a moment. 😀