[events] move write-started from Builder.write to Builder.build method

[chrisjsewell]

This event was recently added in #12567,
but as pointed out in #12680 this may not fire for all builders, since write can be overriden.

I can't recall if there was any additional thinking as to its original placement,
but certainly with #12436, we can "guarantee" that it will be called for all builders when called in build.

[picnixz]

Yes, it's more logic to call it before the write(). However, could you perhaps say in the docs that it's being called before calling write() and thus an overriding of write() should not re-fire that event?

In other words, the write() hook should assume that weite-started has already been fired.

[AA-Turner]

I moved it to .write( as it seems putting the cart before the horse to call it in .build(. Personally, I would close this PR and instead view a failure to emit write-started as a bug and violation of the 'consenting adults' principle (clearly we should document that builders should emit the event).

Is this the only such overidable event?

A

[chrisjsewell]

I moved it to .write(

@AA-Turner 😢, urghh I don't want to moan here, we're all doing good work and mistakes happen, but...
I would urge you not to change people's PRs and merge them, without first asking for feedback from the author (particularly fellow maintainers):
it is not good etiquette, and leads to issues like this that could have been avoided.
I say this also because it is not the first time I've seen it occur, for example #12253

I was just about to integrate this into sphinx-needs, but can't really now, as it has been rendered somewhat useless for its original goal 😞

However, could you perhaps say in the docs that it's being called before calling write()

@picnixz now it is clear why, in https://www.sphinx-doc.org/en/master/extdev/event_callbacks.html, this is already how it is shown; as being called before write:

Personally, I would close this PR and instead view a failure to emit write-started as a bug and violation of the 'consenting adults' principle

But this is never how builders have been documented, and it's not really possible currently to enforce anything on builders in the write method, since it is allowed to be overridden
Plus, as per #12680, we now don't emit this event for half of the internal builders, let alone 3rd-party ones.

(obviously, this was part of the reason for #12436, to at least impose some "constraints" and understanding for developers as to what they can and can't do with builders)

Is this the only such overidable event?

Well, as per https://www.sphinx-doc.org/en/master/extdev/event_callbacks.html#builder-specific-events, there are certainly events that only occur for specific builders

[picnixz]

My stance changes depending on whether #12767 is merged or not:

• If Mark Builder.write() as final #12767 is merged, write-started should be left inside the write().
• If this is not merged, I'd prefer having the write-started event just before the call to write(), otherwise extensions need to explicitly call it (and we need to explicitly add it to the subclasses).