Fix duplicate database change event send on Library.add

[valrus]

Description

Fixes #5560. Also a couple other incidental changes / improvements:

• Add EventType that holds the actual string literals used for event sending. With type checking, this can prevent subtle bugs resulting from misspelled event names.
• Fix test_hidden by using bytestring_path()

To Do

• Documentation.
• Changelog.
• Tests.

[stale]

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[JOJ0]

This looks like a useful bugfix. It looks reasonable to me but I'm a little out of my depth here and since it's rather small to review would ask for your opinions @snejus and @wisp3rwind if you have a minute or two?

[JOJ0]

Thanks for the updates! Please for documentation's sake update the description with the final state. Other than that I think this is good to go :-)

[wisp3rwind]

This looks like a useful bugfix. It looks reasonable to me but I'm a little out of my depth here and since it's rather small to review would ask for your opinions @snejus and @wisp3rwind if you have a minute or two?

I'm also not very familiar with the intricacies of Library. At a quick glance, this change seems correct (In the sense that we don't needlessly duplicate the event; I don't think there is a real guarantee that it is generally impossible for it to be sent in close succession with no actual changes.)

Regarding the test_hidden change, the current (added bytestring_path) version looks good to me.

Small nitpicks:

• I'd prefer the new file not to be named types.py: That's very generic, we already have two or so of these in other places, and I generally find anything called just {t,T}ype{,s} to easily cause confusion regarding what it might refer to. Maybe it could be events.py? Specifically, in the context of Added typehints to the plugins file. #5701, I've been wondering whether it might eventually be possible to provide (optionally) typed events (i.e. in some way specify the arguments that are passed along). That would probably require some additional infrastructure, which would fit well into an events module.
• DatabaseChangeTestBase -> DatabaseChangeTest. Maybe, this should actually be part of test_library.py? In our current tests, we tend to collect tests for a each module in a single file, rather than have one per test case.
• In general, I'd prefer this to be 3 separate PRs, focusing each on only one of the (independent!) parts of this PR. That tends to simplify review, and allows quick merging of uncontentious smaller pieces. Right now, I'm fine with keeping this PR as it is, and merge things in one go.

[valrus]

Thanks for the review! I'll address those nitpicks when I have a chance.

I agree smaller PRs would have been better. This was a case of setting out to fix one thing and stumbling into some other improvements along the way, and by the time I'd done what I actually intended in the first place it was tangled up with the other stuff. I would like to be more disciplined about this and sometimes I am, but it's easier when I'm more familiar with the codebase.

[valrus]

Fixed:

• types.py renamed to event_types.py. I opted for this instead of events.py because it does only have types for now, and I've found that having modules that contain only types decreases the likelihood of circular import snafus because types are unlikely to depend on other things outside the stdlib. It can always be renamed to events.py later if its surface area expands.
• Moved the new test into beets.test.test_library::AddTest and deleted its old location.

[JOJ0]

except conflict resolving, this is good to go. thanks for the input @wisp3rwind , I agree that splitting concerns would be the best approach. this time it's certainly ok like it is.

[JOJ0]

Hi @valrus do you find time to rebase this to current changes? I gave it a rebase-try but a lot of things changed around things you touched and I'm running out of time. You probably are quicker doing it, since you know your changes best. Maybe you want to rebase/squash/drop/re-commit things. The whole history of things that were asked to be changed is not required in the end and will make rebasing easier 👍

[JOJ0]

And appologies for not reacting earlier, which would have prevented a lot of conflicts in the first place. I'm moving my home to a new flat and don't have much FOSS time is the excuse ;-)

[valrus]

@JOJ0 I've rebased it. Sorry for the delay; I was out of town. No sweat about the initial delay; we're all busy :)

[JOJ0]

can you rebase to fewer commits? removing the ones that were not part of the final solution. For example the kwarg you removed in a later commit again? sorry more work but I stumbled across them again while rereading commits one by one. Thanks!!! 🙏

[wisp3rwind]

can you rebase to fewer commits? removing the ones that were not part of the final solution. For example the kwarg you removed in a later commit again? sorry more work but I stumbled across them again while rereading commits one by one. Thanks!!! 🙏

Alternatively, you could squash-merge the PR :)

[JOJ0]

can you rebase to fewer commits? removing the ones that were not part of the final solution. For example the kwarg you removed in a later commit again? sorry more work but I stumbled across them again while rereading commits one by one. Thanks!!! 🙏

Alternatively, you could squash-merge the PR :)

I had that idea but wasn't sure about it because there are at least two different things in this PR. And in case bugs happen later pinpointing to/reverting one commit/thing would be easier. But actually the test hidden fix is not a big deal and most probably correct. So yes I agree let's finally merge it and we all can move on :-)