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

Add commit message merge functionality [Wrong branch] #192

Closed
wants to merge 24 commits into from

Conversation

nlschn
Copy link
Contributor

@nlschn nlschn commented Dec 28, 2020

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

In order to address issue #180 this PR adds functionality to read and process commit message data in "commitMessages.list" files. The first non-white-space line of the message is considered the title, the rest the body of the commit message.
This functionality can be found in "util-read.R"

The ability to merge the commit messages to the commit data is implemented in "set.commits" in "util-data.R" and a new attribute "commit.messages" has been added to "proj.conf" to control what data shall be merged.

For each new functionality tests are provided.

Changelog

  • Add functionality to read and process commit messages in order to merge them to the commit data (See issue Read in and add commit messages #180). Three values are available for the new attribute commit.messages in proj.conf:
    1. none is the default value and does not change the previous behavior of proj.data$set.commits.
    2. title merges the commit message titles (i.e. the first non white space line of a commit message) to the commit data. This gives the data frame an additional column title.
    3. messages merges both titles and message bodies to the commit data frame. This adds two new columns title and message.body.

clhunsen and others added 24 commits December 14, 2017 15:31
v3.0.2 -- Fix minor bug and change output file for showcase
Version 3.1.1

Merged-by: Claus Hunsen <[email protected]>
Version v3.1.2

Merged-by: Claus Hunsen <[email protected]>
Version v3.2

Merged-by: Claus Hunsen <[email protected]>
Version 3.5

Reviewed-by: Claus Hunsen <[email protected]>
Version 3.7

Merged-by: Thomas Bock <[email protected]>
In order to test new functionality (i.e. the read.commit.messages
function) new files containing test commit data were needed.
Add two files containing messages corresponding to the test commit data that
already exists.

See se-sic#180

Signed-off-by: Niklas Schneider <[email protected]>
Add the read.commit.messages and create.empty.commits.list functions
to util-read.R as well as a unit test to test the new functions.
This allows to read 'commitMessages.list' files and return the commit
data separated into message title and body.

See se-sic#180

Signed-off-by: Niklas Schneider <[email protected]>
…reaks

Discussion has shown that codeface separates lines with five spaces, not four.
So the two test files have been modified to account for that fact.

See discussion in se-sic#180

Signed-off-by: Niklas Schneider <[email protected]>
Change the line breaks in the expected output to \n's.

See se-sic#180

Signed-off-by: Niklas Schneider <[email protected]>
Replace five spaces with \n, remove any white space at the beginning and
the end of a commit message.

See se-sic#180

Signed-off-by: Niklas Schneider <[email protected]>
… merge messages into data

Get the commit messsage data using the new read function and merge either nothing, the
title or message and title into the commit.data of the proj.conf instance.

See se-sic#180

Signed-off-by: Niklas Schneider <[email protected]>
Add the new attribute "commit.messages" to the project configuration class with options "none",
"title" and "message" to make it possible to specify what exactly of the commit message data
is to be merged to the commit data.

See se-sic#180

Signed-off-by: Niklas Schneider <[email protected]>
…tles

Add two tests for testing the merge functionality for both full commit messages and
titles only. Fix bug that merges message body instead of title when selecting
option "title"

See se-sic#180

Signed-off-by: Niklas Schneider <[email protected]>
Signed-off-by: Niklas Schneider <[email protected]>
Signed-off-by: Niklas Schneider <[email protected]>
Signed-off-by: Niklas Schneider <[email protected]>
@bockthom
Copy link
Collaborator

Thanks for all the work and also for creating this pull request @nlschn. I did not yet have a look at your implementations, but I just had a look at the list of commits displayed in this pull request—and it seems that your changes are based on the wrong branch (master) as there are lots of versioning merge commits from @clhunsen and myself appearing in this PR. Remember that your implementation should be based on the dev branch. (And if you base your changes onto the current dev branch of se-sic/coronet, there should not be any merge conflict in the NEWS.md file any more, if I am right) However, no worries, we will handle this properly together 😄

But before going into the details of that, one more comment on the merge conflict displayed here: GitHub still says that there is a conflict in NEWS.md— I am not sure if this still is the case or whether this is outdated, but the CI seems to wait for conflict resolution prior to starting the CI pipeline. I can see a button to mark the conflict as resolved, is it only me that can see this button or can you also see that @nlschn ? If so, you may just mark this as resolved (only if it really is resolved), just to see whether the CI pipeline will start then...

And now back to the PR's branch: To base your changes onto the current dev branch of se-sic/coronet, there are several options. First of all, you should add the upstream repository (se-sic/coronet) as a new remote to your local coronet repository. Then please checkout the current dev branch of se-sic/coronet. One idea would be to create a new branch from this dev-branch (you can choose any name you want, for instance niklas-updates, but that is completely up to you) and use this as your main working branch. Another option would be to just update your dev branch with the changes from the upstream dev branch. In both cases, you could just cherry-pick the commits from this PR which you want to keep into your new working branch. But this is just a suggestion, there are also other ways to achieve that your current branch is based on dev (e.g., via interactive rebase), but the above mentioned options sound easier to me. In a nutshell, here we just have are some technical details in your branch setup we have to deal with—we can also do that in a hands-on meeting in January. There is no need to do this right away, we can also fix that after we will finally have reviewed your pull request (remember that I did not yet have a look at the commits' contents). But you can already start familiarizing yourself with the different options and advanced capabilities of git 😉


Let me add one comment for @clhunsen: GitHub's dashboard looks weird to me. In today's activity feed, I can see a comment from you on this PR mentioning a log message, but this comment is not displayed in this PR—even clicking on the comment in my activity feed just brings me to the initial comment of this PR. Do you know what has happened to your comment?

@nlschn
Copy link
Contributor Author

nlschn commented Dec 28, 2020

I did not want to resolve the conflict with this conflict button, because I didn't know if I this generated commit can be signed off.

Thanks for your suggestions, I will try them out.

@nlschn nlschn closed this Jan 7, 2021
@nlschn nlschn changed the title Add commit message merge functionality Add commit message merge functionality [Wrong branch] Jan 7, 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.

3 participants