First attempt at fixing #3132.

[djl]

Add a new no_clobber config option that contains a list of "miscellaneous" metadata fields not to be overwritten by empty values.

I'm not entirely happy with the no_clobber name. Suggestions welcome.

[sampsyo]

Looks good! I just have a couple of small concerns about the name and the role this plays:

• Putting a field on this list doesn't prevent clobbering altogether—it only prevents clobbering when the new value would be empty. So the name could mislead people into thinking that putting a field there "protects" it completely (i.e., never overwrites that field with anything).
• There are other fields that have is not None checks in this function that aren't in the miscellaneous metadata list. For example, disctitle gets handled separately (for some reason), as do many other fields. So putting that field in this list won't have the intended effect.

[sampsyo]

A type assertion here might be useful: .as_str_seq() instead of .get().

[djl]

Good catch. Fixed.

[djl]

Putting a field on this list doesn't prevent clobbering altogether—it only prevents clobbering when the new value would be empty. So the name could mislead people into thinking that putting a field there "protects" it completely (i.e., never overwrites that field with anything).

Great point. I'm open to any suggestions for a better name. nullable_fields or something, perhaps?

There are other fields that have is not None checks in this function that aren't in the miscellaneous metadata list. For example, disctitle gets handled separately (for some reason), as do many other fields. So putting that field in this list won't have the intended effect.

Also a good point. Should all these fields be moved into the no_clobber list? They're track fields so they'll need to handled separately from the album fields, making no_clobber slightly more complicated.

[sampsyo]

Hmm... I’m not sure what the best course of action is. Maybe it would be simplest to invert the sense of the option (overwrite_null?), leave it empty by default, and possibly leave it undocumented for now because of its hard-to-explain limitations?

[djl]

Updated the code to handle the track-level fields and inverted the logic as asked. Also removed the documentation and changelog entries for now.

And it looks like Travis is having a bad day.

[sampsyo]

Looking good; thanks! Travis seems like it's pointing out a very very nitpicky style complaint:
https://travis-ci.org/beetbox/beets/jobs/484712974#L1079

[djl]

All green 👍

[sampsyo]

Awesome! This looks like it will be useful to build on, and it made the code for that function simpler. Thanks for your help with this!! ✨