mila-iqia / milatools

Tools to connect to and interact with the Mila cluster
MIT License
63 stars 12 forks source link

Add type annotations and more tests #61

Closed lebrice closed 11 months ago

lebrice commented 1 year ago

Note: I'm feeling less and less comfortable with the master branch being deeply modified (with the new _* default funcs). It doesn't change anything for the end user and prevents us from quickly bringing a fix to users or simply fix a dependency version. But we're too far in the changes now

Hey @satyaog, @breuleux thanks for the reviews, sorry I didn't reply earlier.

I just want to emphasize something: This PR doesn't change anything about the behaviour of the code. The type hints that are added describe the types and signatures of the methods as they are currently.

As for the unit tests, I guess having meh integration tests would be much better than having no tests at all. I think making changes to the master branch to introduce tests actually really aligns with your point: We don't want breaking changes to join master. The problem is, until we add some tests, we have no idea what's breaking or not. We need to take a first jump at some point.

lebrice commented 11 months ago

Closing in favour of #76 and #75 (as well as a lot of overlapping changes with the now-merged #74 )