Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Satisfy mypy complaints due to nexus merge #94

Merged
merged 4 commits into from Mar 10, 2023
Merged

Conversation

glatterf42
Copy link
Member

@glatterf42 glatterf42 commented Mar 7, 2023

Fix the errors mypy throws due to the recent merger of the nexus/water module.

These errors seem to fall into two broad categories: the classes Annotation and Code from sdmx1 require InternationalStrings to be passed as text arguments. I have not yet worked with them, so I don't know for sure that I kept the intended behaviour. Secondly, several functions from the dataclasses module require arguments to be a DataclassInstance or of Type[DataclassInstance] or both. Curiously, the dataclasses-provided functionis_dataclass doesn't just return True when dealing with a DataclassInstance, but also when dealing with a Dataclass itself. So passing this function is not enough to ensure we're dealing with a DataclassInstance. Again, I've fixed the conditions so that the correct data types are provided (in util/config.py and util/common.py), but I don't know if I kept the intended behaviour. E.g. in config.py, the replace function is the first one to actually require existing to be a DataclassInstance, but maybe that check would also make sense before, i.e. when is_dataclass is called.

How to review

Please check that I kept the intended behaviour.

PR checklist

  • Continuous integration checks all ✅
  • [ ] Add or expand tests; coverage checks both Only type-hint related changes intended
  • [ ] Add, expand, or update documentation. Only type-hint related changes intended
  • [ ] Update doc/whatsnew. Only type-hint related changes intended

@glatterf42 glatterf42 self-assigned this Mar 7, 2023
@codecov
Copy link

codecov bot commented Mar 7, 2023

Codecov Report

Merging #94 (035106d) into main (30d03b3) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #94      +/-   ##
==========================================
+ Coverage   66.80%   66.83%   +0.03%     
==========================================
  Files          58       58              
  Lines        3973     3977       +4     
==========================================
+ Hits         2654     2658       +4     
  Misses       1319     1319              
Impacted Files Coverage Δ
message_ix_models/util/cache.py 100.00% <100.00%> (ø)
message_ix_models/util/config.py 100.00% <100.00%> (ø)

@khaeru
Copy link
Member

khaeru commented Mar 8, 2023

Per the InternationalString errors, these are upstream. Can you please check with the branch from khaeru/sdmx#117? If that causes them to disappear, we can avoid the respective changes here.

@khaeru khaeru added the ci Continuous integration & testing label Mar 10, 2023
Copy link
Member

@khaeru khaeru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fixes!

@khaeru khaeru merged commit 0c6fa02 into main Mar 10, 2023
15 checks passed
@glatterf42 glatterf42 deleted the fix_nexus_mypy_issues branch March 10, 2023 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Continuous integration & testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants