Add issue template with checklist for bumping the minimum required GMT version#1630
Add issue template with checklist for bumping the minimum required GMT version#1630
Conversation
maxrjones
added
maintenance
| - [ ] Update `test_get_default` in `pygmt/tests/test_clib.py` | ||
| - [ ] Update compatibility table in `README.rst` | ||
| - [ ] Fix failing tests (1 or more PRs) | ||
| - [ ] Remove [xfail](https://docs.pytest.org/en/stable/skipping.html#xfail-mark-test-functions-as-expected-to-fail) pytest markers on tests that are now xpass |
There was a problem hiding this comment.
Should this be under the "Fix failing tests" header?
There was a problem hiding this comment.
I think this should stay separate because these tests do not get listed under the failures, so people will need to pay attention to the full list. For example, here are the failing tests using 6.3 and the three xpasses are not listed:
Co-authored-by: Will Schlitzer <schlitzer90@gmail.com>
Co-authored-by: Dongdong Tian <seisman@users.noreply.github.com>
| - [ ] Update `.github/workflows/cache_data.yaml` | ||
| - [ ] Update `.github/workflows/ci_docs.yml` | ||
| - [ ] Update `.github/workflows/ci_tests.yaml` |
There was a problem hiding this comment.
Probably a little off-topic: The "minimum required GMT version" and "the GMT version used in CI" may be different if we decide to support multiple GMT versions in the future. We probably need to discuss how to test PyGMT with multiple GMT versions when we are close to PyGMT v1.0.0.
There was a problem hiding this comment.
Good point. This could get tricky to manage, so I think it's a good issue topic.
seisman
left a comment
seisman
left a comment
There was a problem hiding this comment.
The template looks good to me.
seisman
added
the
final review call
| - [ ] Update `required_version` in `pygmt/clib/session.py` | ||
| - [ ] Update `test_get_default` in `pygmt/tests/test_clib.py` | ||
| - [ ] Update compatibility table in `README.rst` | ||
| - [ ] Fix failing tests (1 or more PRs) |
There was a problem hiding this comment.
'Fix failing tests' is listed after 'Bump the minimum GMT version'. Does this imply that we are ok with the CI tests breaking on the main branch for a period of time?
There was a problem hiding this comment.
I think the only way the tests wouldn't fail for a period of time is if all the changes were in a single PR or xfails were added just to be removed shortly after. I would prefer to have the tests break for a short bit in order to split up the tasks and reduce efforts, but of course other opinions are welcome.
There was a problem hiding this comment.
Guess it depends on the situation (i.e. how many tests are breaking, and how big of a fix is required). Having failing tests on the main branch means that PRs will need to be merged by those with admin permissions (currently 5, maybe 3 active), so we definitely want to keep that period as short as possible.
There was a problem hiding this comment.
Good point. I agree that we'll want to make the tests pass again as quickly as possible. We can also adjust the issue checklist from what is in the template on a case-by-case basis.
maxrjones
removed
the
final review call
…T version (GenericMappingTools#1630) * Add issue template with checklist for bumping the minimum required GMT version Co-authored-by: Will Schlitzer <schlitzer90@gmail.com> Co-authored-by: Dongdong Tian <seisman@users.noreply.github.com>
4 participants
Description of proposed changes
Bumping the required GMT version is often an involved process with multiple PRs, but infrequent enough (~2/year) that it's easy to forget all the changes needed. This PR proposed an issue template with a checklist for common steps needed for bumping the required GMT version after a GMT release.
Reminders
make formatandmake checkto make sure the code follows the style guide.doc/api/index.rst.Slash Commands
You can write slash commands (
/command) in the first line of a comment to performspecific operations. Supported slash commands are:
/format: automatically format and lint the code/test-gmt-dev: run full tests on the latest GMT development version