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

Add more type checks to core API #700

Closed
jodal opened this Issue Feb 17, 2014 · 5 comments

Comments

2 participants
@jodal
Member

jodal commented Feb 17, 2014

Example of problem we currently have: Web clients can easily get strings into the tracklist by calling mopidy.tracklist.add('foo'), where 'foo' is handled as a list of tracks, adding 'f', 'o', and 'o' to the tracklist. Obviously this fails and crashes Mopidy when someone try to use the strings as Track objects.

We should probably do a lot more isinstance() checks where we expect Mopidy models, so we can fail earlier and ease debugging.

@jodal jodal added this to the Core cleanup milestone Jun 22, 2014

@adamcik

This comment has been minimized.

Member

adamcik commented Jul 26, 2014

Other idea relevant to this is making the model fields typed.

@adamcik

This comment has been minimized.

Member

adamcik commented Apr 5, 2015

I've done a quick experiment adding:

  • isinstance checks for models and uris
  • bounds checks for numbers
  • coercing to bools where applicable

Result has been that I've only broken one test that tries to do a negative seek for some reason.

Based on my quick experience I'm suggesting the following:

  1. isisntance type checking for all models
  2. bounds checking for numbers where reasonable
  3. coercing of booleans such as tracklist options which get stored
  4. is_valid_uri checking for all URIs
  5. is_valid_query checking / normalization for all queries, criteria etc.
  6. is_valid_state check for set_state

This should cover most of the APIs in core that can be called with arguments.

@jodal

This comment has been minimized.

Member

jodal commented Apr 6, 2015

This sounds like a good plan. Make sure to raise ValueError and TypeError as appropriate, and not simply AssertionError.

@adamcik

This comment has been minimized.

Member

adamcik commented Dec 5, 2015

Haven't I already done most of this?

@jodal jodal modified the milestones: v1.1 - Robust core, v4.0 - Core cleanup Dec 5, 2015

@jodal

This comment has been minimized.

Member

jodal commented Dec 5, 2015

Yes, you have.

@jodal jodal closed this Dec 5, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment