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

Nullity of fields #14

Open
stevenwdv opened this issue Jan 18, 2021 · 4 comments
Open

Nullity of fields #14

stevenwdv opened this issue Jan 18, 2021 · 4 comments

Comments

@stevenwdv
Copy link

I got back to developing an application using this API and while refactoring so that it works with the update I noticed that all fields are suddenly explicitly nullable. Even e.g. IRecording.Title is nullable, why it can't actually be null, right? Why is this? It makes programming with the API a bit confusing as now I have to add !s everywhere or check for nullity when it will never be null.

PS: Thanks for maintaining this project!

@Zastai
Copy link
Owner

Zastai commented Jan 18, 2021

In part it will be because I enabled nullable reference types for the code base, Which meant that any field where I could not guarantee a non-null value had to be nullable, to avoid warnings from the compiler.

For IRecording.Title specifically, the Title comes in via ITitledEntity, and I_think_ this was used both with entities that require the title, and those that don't. So it had to be nullable. Changing this (either by dropping ITitledEntity or adding IOptionallyTitledEntity) would be an API break, and I try to avoid those.

Note that you can do things like var title = recording.Title ?? ""; and var titleLength = rec.Title?.Length ?? 0;. That avoids having to do explicit foo != null in many cases.

@stevenwdv
Copy link
Author

Ah ok. It's just that it feels silly to put null checks or e.g. ?? new[]{} (or "") everywhere when things can never be null. It just makes code a lot messier. And putting !s also doesn't make it prettier and I'm not sure which things can actually be null.
Speaking about that: do you have a resource I can use to check which types can actually be null in a certain context? The MusicBrainz docs I found seemed a bit abstract.

[...] would be an API break, and I try to avoid those.
But wasn't the whole nullability update etc. an API break? Anyway, I personally prefer clear APIs to keeping with a suboptimal interface, but I understand the reasoning.

I hope that maybe you can reconsider this, but I can't force you ;-)

@Zastai
Copy link
Owner

Zastai commented Jan 22, 2021

Another thing is that I've been bitten in the past by the fact that the JSON serialization on MusicBrainz is... not very standardized when it comes to empty fields. Sometimes you get an explicit '', sometimes an explicit null, and sometimes the field is omitted; and this has changed between releases sometimes.

I suppose I could explictly attempt to not have nulls anywhere, and just map things to 0 / "" / Array.Empty<> myself. On the assumption the latter only involves one instance per type, that would not even cause many extra allocations. I'd just have to take care there's no properties where 0 and "not set" have different meanings.

@Zastai
Copy link
Owner

Zastai commented Jan 22, 2021

And obviously fields that are other entity types would need to remain nullable if they're optional, because there's no safe no-allocate "default" value for those.

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

No branches or pull requests

2 participants