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

Get mypy working again #1457

Merged
merged 7 commits into from
Jan 13, 2025
Merged

Get mypy working again #1457

merged 7 commits into from
Jan 13, 2025

Conversation

gaffney2010
Copy link
Member

Resolves #1406

@gaffney2010
Copy link
Member Author

Need to test adding new exception handling. Will reopen.

@gaffney2010 gaffney2010 reopened this Nov 16, 2024
@drvinceknight
Copy link
Member

Looks like this is failing on coverage:

axelrod/load_data_.py                                               37      1    97%   30

@gaffney2010
Copy link
Member Author

Looks like this is failing on coverage:

axelrod/load_data_.py                                               37      1    97%   30

But why? That line is tested. Any idea?

@drvinceknight
Copy link
Member

But why? That line is tested. Any idea?

Could it be the assertRaisesRegex doing something funky? Would it be worth trying assertRaises? That's a very underwhelming idea that I'm not convinced by myself.

@gaffney2010
Copy link
Member Author

Could it be the assertRaisesRegex doing something funky? Would it be worth trying assertRaises? That's a very underwhelming idea that I'm not convinced by myself.

I actually had assertRaises that I changed thinking perhaps it wanted to test the text of the error message.

@gaffney2010
Copy link
Member Author

Ah, so get_data raises FileNotFoundError in my test case, but returns None for some other kinda failure. I'll figure it out.

@gaffney2010
Copy link
Member Author

I chose to inject the dependency on get_data for the sake of testing because:

  1. It's a little hard to figure out this failure path
  2. It seems to be missing tests even in the core library.
  3. It may require the library to be formatted different (whatever PEP 302 is)

@gaffney2010
Copy link
Member Author

Oh, mypy is giving a lot more errors. I'm not sure why I thought it was resolved. Will reopen after addressing the others.

@gaffney2010 gaffney2010 closed this Dec 9, 2024
@gaffney2010 gaffney2010 reopened this Dec 9, 2024
@gaffney2010
Copy link
Member Author

Oh, mypy is giving a lot more errors. I'm not sure why I thought it was resolved. Will reopen after addressing the others.

Ah, this was passing locally. I'm gonna continue to test via CI / build

@gaffney2010
Copy link
Member Author

Everything seems to be passing now.

@drvinceknight
Copy link
Member

I can't seem to get any information from read the docs about the failed build there.

CleanShot 2024-12-10 at 12 48 21@2x

Do the docs build locally for you? If they do it could be a read the docs thing instead of a sphinx thing...

@gaffney2010
Copy link
Member Author

Do the docs build locally for you? If they do it could be a read the docs thing instead of a sphinx thing...

python doctest.py runs without errors

image

Is there a different command to build?

@marcharper
Copy link
Member

marcharper commented Dec 30, 2024

That's just the doc tests, you need to also build the docs by running make. I thought we had this documented but I wasn't easily able to find it just now.

@drvinceknight
Copy link
Member

That's just the doc tests, you need to also build the docs by running make. I thought we had this documented but I wasn't easily able to found it just now.

You're right Marc, that's not documented at all. We should put it here right? https://axelrod.readthedocs.io/en/stable/how-to/contributing/running_tests.html#testing-the-documentation.

I believe:

cd docs
make html

is what is needed.

@gaffney2010
Copy link
Member Author

make html runs locally and creates no new files.

image

When we update the docs for make html, can we specify which version of Sphinx we're using - I had compatibility issues.

@marcharper
Copy link
Member

@drvinceknight yes that seems like a good place.

Where are the commands that CI is running to build the docs?

@drvinceknight
Copy link
Member

It's using the readthedocs config file which is here: https://github.com/Axelrod-Python/Axelrod/blob/dev/.readthedocs.yaml

Here's info about the file itself: https://docs.readthedocs.io/en/stable/config-file/v2.html

The rest is essentially done through the readthedocs configuration panel. As this isn't changing anything relevant to the docs how about we merge it, then I'll open a PR to fix the docs build?

@drvinceknight
Copy link
Member

The fact that the other recent PRs pass rtd but not this one is weird...

@drvinceknight drvinceknight merged commit 6d2d465 into Axelrod-Python:dev Jan 13, 2025
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI failing due to typing issues
3 participants