carpentries-incubator / python-intermediate-development

"Intermediate Research Software Development Skills (Python)" Lesson Material
https://carpentries-incubator.github.io/python-intermediate-development/
Other
44 stars 51 forks source link

pylint-ci branch committed to but never merged #365

Open sjvrijn opened 3 weeks ago

sjvrijn commented 3 weeks ago

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 ```console $ 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