Our First Pull Request for kworkflow

After receiving feedback and making adjustments, our first pull request for kworkflow was accepted. This blog post describes the process and lessons learned. You can find the pull request’s discussion here.

Issues with Readability

The initial feedback we received pointed out some minor issues, such as poorly named files and specific case tests. These were not “feature-breaking” but highlighted the need for better readability and clarity in our code. At first, these issues seems to be all code-styling related and maturity in coding.

One case to mention is the part of the code reponsible for capturing the content inside the MODULE_AUTHOR macro, trimming and formatting the content to be printed by the function.

This block of code is actually a single line of commands piped one after another and is quite hard to understand what is happening. The suggestion was breaking this huge commands into a small set of separeted blocks and adding some comments to improve readbility and documentation of the changes and features.

Understanding and modifying

As we improved our pull request based on the suggestions, we questioned the purpose of the following code:

sed --expression ':a' --expression 'N' --expression '$!ba'

Despite our research, we couldn’t determine what this code did. We reviewed the sed documentation and the original commit when –authors was introduced but found no clear answers. Removing this code didn’t affect the output.

Eventually, @davidbtadokoro explained that this code aimed to handle multi-line author statements, which was precisely what we were trying to address with our pull request. His detailed explanation is in the PR thread.

Updating Our PR

After receiving initial change requests, we added new commits to improve our code. The maintainers then asked us to squash these commits into one:

$ git rebase --interactive HEAD~4

We selected:

    pick ...First Commit...
    squash ...Second Commit...
    squash ...Third Commit...
    squash ...Fourth Commit...

This merged the new commits into a single one. We then updated our remote fork’s branch:

$ git push --force-with-lease origin issue69

where issue69 is the branch we created for this issue.

Final touches and PR resolution

We enhanced our code’s readability by splitting it into sections and adding comments. We also removed the “unknown” code as it wasn’t necessary for our new function. Additionally, we created a new test file covering the multi-line use of MODULE_AUTHOR.

There were further discussions on code style and best practices, but they are not crucial for this blog post. For full details, see the discussion here.

Changes merged into kw’s upstream

Our changes were accepted and merged into the kworkflow upstream. They were first merged into the unstable branch and will be periodically merged into the master branch.

You can find our contribution here.