Better handling of list of IDs when there are more than 50

[artyuum]

I found out that when providing more than 50 tracks, the other tracks will be silently ignored: https://github.com/rinukkusu/spotify-dart/blob/f86276b49510cff05de762ad4c3f1a9f7e3ee1cb/lib/src/endpoints/tracks.dart#L77-L81

Maybe it would be better if we throw an error instead? WDYT?

[hayribakici]

@artyuum Something like:

Future<void> save(List<String> ids) async { 
   assert(ids.isNotEmpty, 'No track ids were provided'); 
   assert(ids.length <= 50, 'Track id length exceeded');
// ...
}

?

[artyuum]

Doesn't assert() only work in development? This means once compiled these asserts will be disabled 🤔

I'm not familiar with the Dart language but I read it here: https://dart.dev/language/error-handling#assert

I would keep this check every time, no matter the env, and I would also explicitly tell about this limit of 50 in the error message so the dev knows exactly why this error happened.

[hayribakici]

oh okay, I didn't know about this. This also means, that all the other asserts are practically useless, once the library is compiled.

@rinukkusu I wonder now, what the use cases of the assert statements might be if they are ignored. Is it only for debug purposes?

Plus, if we throw exceptions e.g. for empty ids or exceeded ids-length, the users of this library then have to catch them. This would indicate "breaking changes" in the library, which imho I would avoid.

Having this in mind, I would suggest to return a Future.error() with some ArgumentErrors (or we create our own). So something like

Future<void> save(List<String> ids) async {
   if (ids.isEmpty) {
      return Future.error(ArgumentError('No track ids were provided'));
   }
   if (ids.length > 50) {
      return Future.error(ArgumentError('Number of track ids exceeds limit of 50'));
   }
   // ...
}

@rinukkusu what do you think?

[artyuum]

This would indicate "breaking changes" in the library, which imho I would avoid.

No need to completely avoid it. Such change can be done in a new major version (v1).

[rinukkusu]

@rinukkusu I wonder now, what the use cases of the assert statements might be if they are ignored. Is it only for debug purposes?

@hayribakici I don't really remember - I think this is something that got introduced by other collaborators like yourself. 🤔

Having this in mind, I would suggest to return a Future.error() with some ArgumentErrors (or we create our own). So something like

@hayribakici from what I could gather, it looks like returning Future.error(...) and throwing exceptions behave the same. Both can be caught with a try-catch block or in the .catchError(...) method of the Future. That's why I'd vouch for throwing exception objects like ArgumentError directly instead of returning Future.error(...). A little less fluff.

I don't think it's that breaking anyway, since if we don't "protect" from it, in most cases we'll just get an HTTP error then from the Spotify API, if not undefined behavior, right?

Relevant xkcd 😸

[hayribakici]

@hayribakici I don't really remember - I think this is something that got introduced by other collaborators like yourself. 🤔

tbh, I just went with it 😅

Both can be caught with a try-catch block or in the .catchError(...) method of the Future. That's why I'd vouch for throwing exception objects like ArgumentError directly instead of returning Future.error(...). A little less fluff.

okay, sounds good 👍

@rinukkusu should we create a specific issue for this, that indicates all assert statements?

[rinukkusu]

@hayribakici yeah, a new issue sounds sensible. Thanks!

[hayribakici]

Closing, since we created another issue for this.