Closed macflo8 closed 4 months ago
@macflo8, this and #188 look good, thank you!
One thing I notice is that the commit messages do not conform to our code style (see the first bullet).
For this -tidy
branch with a few commits, it is easy to git rebase -i
and use the r
/reword
command to adjust.
@macflo8, I've migrated the required functions now and cleaned up several imports. However, there's still work left to be done: the code quality check (as you analyzed) should be the first, but you also have a doc.rst
file in message_ix_models/model/material
. These files should generally live under doc/
, in this case probably doc/material
(since we also have doc/water
. This file in particular is calling automodule
for several message_data.model.material
functions. These should all be migrated now, so please adapt to the new locations.
Attention: Patch coverage is 11.36108%
with 1576 lines
in your changes missing coverage. Please review.
Project coverage is 52.2%. Comparing base (
9d9fa6c
) to head (cc2e59f
).
Thanks, @glatterf42 for taking care of that 🙏🏻 I will have a look at the materials docs.
The materials doc file is now moved and the code quality checks are passing. Do you agree to merge this PR @khaeru @glatterf42 ?
Looking at step (9) in our migration guide:
- Merge the clean-up branch from (7) into (6), and then (6) into
main
.
… I see the following things:
migrate-materials_2024-W17
on to main
, and then rebasing this migrate-materials_2024-W17-tidy
on to migrate-materials_2024-W17
.Per my earlier comment about commit messages, there are still some like:
(Many others still use the passive voice, like “Sets added”, but I guess this could be tolerated.)
Once we address these items, then we can make a simultaneous approval of both PRs, and then perform step (9).
Thanks @khaeru!
- I was not sure about the "Update doc/whatsnew". Should we update this?
Please have a look at these past commits to the file, particularly this one and this one. Something similar and short should be fine.
Please add the commit to this branch; then it will automatically be included for #188 as well.
Thanks, this is looking good now. Please open a new issue to mention and track that the materials team will provide tests at a later date (ideally within a few weeks or months).
@khaeru, would we want to exclude the new files from coverage (to keep the test coverage apparently high) or are we fine with the reduction in coverage (as we were for the nexus module)?
Either way, I think we can then approve this PR and merging this branch to the first migration branch hopefully resolves the code quality issues there, too.
@khaeru, would we want to exclude the new files from coverage (to keep the test coverage apparently high) or are we fine with the reduction in coverage (as we were for the nexus module)?
I think we ought to be honest, so better to accept the reduction.
The tracking issue for tests and other pending items is now linked in the PR checklist. I will merge the PR once you approve @glatterf42 @khaeru.
This separate branch includes commits to tidy and make MESSAGEix-Materials run on message-ix-models.
How to review
PR checklist
~- [ ] Continuous integration checks all ✅~ Not possible due to missing tests