remove 'final' decorator from method which is overriden in subclass

[danieleades]

trafficstars

this method is marked as 'final', but then overridden in a subclass. Which doesn't make a lot of sense

[picnixz]

Actually, it should be final for the user but not final for us. It's not meant to be overridden by a user. We had the discussion with Chris in the PR adding this final I think (but we should have added a comment for the type-ignore).

[danieleades]

Actually, it should be final for the user but not final for us. It's not meant to be overridden by a user. We had the discussion with Chris in the PR adding this final I think (but we should have added a comment for the type-ignore).

maybe it's just me, but that seems non-idiomatic.

yes, a comment on the ignore would have helped a lot!

I've added an alternative formulation for consideration. Another pattern worth considering would be to expose a 'pre-build' hook, but not the build method itself

class Builder:
    # ...
    def _pre_build(self) -> None:
        """Run a callback before calling 'build'."""
        pass

    @final
    def build(
        self,
        docnames: Iterable[str] | None,
        summary: str | None = None,
        method: Literal['all', 'specific', 'update'] = 'update',
    ) -> None:
        """Main build method, usually called by a specific ``build_*`` method.

        First updates the environment, and then calls
        :meth:`!write`.
        """
        self._pre_build()
        # existing build logic ...

class MessageCatalogBuilder(I18nBuilder):
    # ...
    def _pre_build(self) -> None:
        self._extract_from_template()

[picnixz]

I prefer that approach @chrisjsewell what's your opinion on that matter?

[AA-Turner]

Adding an entirely new _build method for one instance that needs overriding seems overkill?

I think we can live with the override, or find some way to refactor MessageCatalogBuilder.build().

A

[picnixz]

that needs overriding seems overkill?

Yes. But it seems a bit like "hey, don't do this but we do it". But maybe we should add a comment first and then decide on how to refactor it (might be hard to refactor though because I don't see how we could do it).

[chrisjsewell]

Adding an entirely new _build method for one instance that needs overriding seems overkill?

yeh I tend to agree,

I think the decorator should definitely not be removed, because as discussed in https://github.com/sphinx-doc/sphinx/pull/12436, it is critical to the functioning of sphinx that all builders "respect" this method and don't change its code.

Obviously this is a relatively edge case in that It's not really "overriding" the method, it's just "appending" some code to the start, unfortunately there is nothing in python typing to say "if you override, you have to call super" which is kind of the main point here

[picnixz]

Actually an alternative could be "build-started" which would be called juuuust before calling .build(), a bit like your "write-started" (I hope we don't have this event yet... otherwise we should think of a better name...)

[AA-Turner]

I think this is superseded by the change to Builder.write() in 8.1.