On Wed, 2024-03-20 at 18:32 +0100, Abderrahim Kitouni wrote: > Hi, > > Le mer. 20 mars 2024 à 13:24, Jürg Billeter <[email protected]> a écrit : > > [...] > > > # Returns the number of URLs the mirror can provide for `alias`, zero if > > > none. > > > def get_n_mirrors_for_alias(self, alias: str) -> int > > > > If the support for multiple URLs is purely for compatibility with > > existing non-plugin mirror definitions and we think it's not useful > > with mirror plugins (i.e., an independent mirror plugin instance should > > be configured instead), I'd rather keep this out of the public plugin > > API. > > I think it is. I always found it weird that a single mirror can > provide more than one url for a given alias, when we can provide more > than one mirror. But I'm open to more opinions.
I tend to agree but I'm not sure whether there may be a use case I'm forgetting. > > > One option would be to make the above method internal and make the > > public method return a bool instead. Another option may be for the > > public method to return a list of alias strings, not sure which variant > > is a better fit. > > A third variant which I was considering earlier is to have a method of > SourceMirror that plugins could call during their configure() to set > the supported aliases. This way, plugins would still only implement > translate_url(). This sounds reasonable to me as well. Either approach works for me. > > > > # Return the `mirror_idx`th URL to use for downloading `source_url`. > > > # Any extra data that needs to be passed to Source implementations > > > # can be added to the `extra_data` dict. > > > def translate_url( > > > self, > > > alias: str, > > > source_url: str, > > > mirror_idx: int, > > > extra_data: Dict[str, Any], > > > ) -> str > > > > Here I'm leaning towards introducing some kind of translation object as > > parameter to provide maximum flexibility. Besides making it possible to > > provide access to the mirror index (or alias substitute URL) only via > > an internal method or attribute, this would also make it possible to > > support translation of unaliased URLs in the future, without having to > > mark `alias` as an optional parameter. > > I'm not sure I see how it would be different in the case of no alias? > The way I see it is that `TranslationObject.get_alias()` would have an > `Optional[str]` as return type rather than `alias` being > `Optional[str]`, which is essentially the same thing (if we change it > during in a point release it would be a bit awkward as the plugin > suddenly needs to handle it. I see it as valuable indeed if we ever > wanted to give more information to plugins though. My point was the overall improvement in flexibility. It should make it easier to extend without breaking compatibility with existing plugins. Cheers, Jürg
