pylint-ci branch committed to but never merged

[sjvrijn]

At the end of the 'Diagnosing Issues and Improving Performance' episode, the final git instructions lead to a confusing situation, where a new pylint-ci branch is created from develop and committed to, but is never merged into develop or test-suite. This causes confusion in later episodes when the CI does not work as expected.

Git history at this point in the episode

$ git log --oneline --graph --all -n6
* dddd639 (HEAD ->test-suite, origin/test-suite) add GA build matrix for OS and Python version
* 92fe5b7 add GitHub Actions config
* e641beb add coverage support
* 5010c79 add parameterisation mean/min/max test cases
* d053f77 add initial test cases for daily_min/max()
* 7ddd4ca (origin/style-fixes, origin/develop, style-fixes, develop) add pylint to requirements

Condensed instructions from the last two sections in the episode:

Improving Robustness with Automated Code Style Checks

$ git switch -c pylint-ci develop          # 'sidestep' current test-suite branch
# add pylint step to .github/workflows/main.yml
$ git add .github/workflows/main.yml
$ git commit -m "Add Pylint run to build"  # commit made to pylint-ci branch
$ git push origin test-suite               # no new commits pushed

Merging to develop Branch

$ git switch develop
$ git merge test-suite                     # merge previous commits from test-suite, but nothing from pylint-ci
# assuming there are no conflicts
$ git push origin develop

Git history at the end of the episode:

$ git log --oneline --graph --all -n7
* d879f7b (pylint-ci) add pylint run to build
| * dddd639 (HEAD -> develop, origin/test-suite, origin/develop, test-suite) add GA build matrix for OS and Python version
| * 92fe5b7 add GitHub Actions config
| * e641beb add coverage support
| * 5010c79 add parameterisation mean/min/max test cases
| * d053f77 add initial test cases for daily_min/max()
|/
* 7ddd4ca (origin/style-fixes, style-fixes) add pylint to requirements

I see a couple of options, but what would the preferred order of operations be here?

1. Add explicit instruction to merge pylint-ci into develop, separately from merging test-suite into develop
2. Merge pylint-ci into test-suite before the merge into develop
3. Omit the dedicated pylint-ci branch and simply commit to test-suite instead

[douglowe]

It looks to me like the test-suite branch isn't merged into develop at the end of the Continuous Integration for Automated Testing episode. Perhaps the intention was to have the test-suite branch merged into develop at the end of this lesson instead? @anenadic & @steve-crouch - was this your intention?

If this is the case then I think we go with option (3) - replacing git switch -c pylint-ci develop with git switch test-suite (to make sure we're on the correct branch), and leaving everything else as it is now.

[sjvrijn]

It looks to me like the test-suite branch isn't merged into develop at the end of the Continuous Integration for Automated Testing episode.

This exact issue just came up during a workshop, so I think that should definitely be the case

[bielsnohr]

Okay, I think I have tracked down the source of the problem. It is the introduction of the pylint-ci branch in Episode 2.4. All of the problems here along with those raised by #376 can be resolved if we just don't bother creating the pylint-ci branch, keep using the test-suite branch, and then merge that branch into develop as detailed in this section at the end of Episode 2.4.

@sjvrijn did you or your instructors not see that section? It is understandable because it is buried at the bottom of quite a long lesson. We could perhaps consider turning it into its own episode so that it is harder for learners to miss. For example, if you decide as a class to skip the content on using Pylint, then it would be easy to miss those steps. I will wait for your feedback on that, and we can consider the move to a separate episode based on that.

[sjvrijn]

@bielsnohr It's that exact section that causes the confusion, since it explicitly instructs to merge test-suite into develop, indeed leaving the commit on pylint-ci hanging, causing the problems later on. Not creating the pylint-ci branch in the first place seems like the simplest solution, so I fully agree with #376 .