Breaking changes

[benbovy]

@abkfenris IIUC with #146 Rest won't work anymore with a single dataset and SingleDatasetRest (or the accessor) must be used instead? That's good IMO but are we ok with this breaking change? Xpublish is still at an early stage of development but I don't know to which extent it is used currently (it has been there for some time already).

Alternatively, perhaps it is possible to go through a smoother (short) transition using some Rest.__new__ hack? I don't think it is really worth it, but at least we might want to add a clear error message suggesting to use SingleDatasetRest when trying to pass a single Xarray Dataset to Rest.

Are there other breaking changes? I haven't seen any but I haven't looked in-depth at #146 yet.

[abkfenris]

Whoops, I thought I had highlighted that for discussion to figure out the right track, but I guess I did that on #145.

Correct, a single dataset shouldn't play nice anymore with Rest and will need to use SingleDatasetRest or .rest.

I'm not sure how often Rest is being used for single datasets, except for in the instance of the accessor. From how I've used Xpublish, and the few folks that I've talked to, it seems like most direct usage of Rest is for a dict of datasets rather than single ones, but without having a better handle on users, as we say here in Maine it's hard tellin, not knowin.

I'd prefer to throw an error in Rest.__init__ over getting creative with Rest.__new__. __new__ is something that most folks rarely touch, so I think it's a lot less understandable.

https://github.com/xarray-contrib/xpublish/blob/ce9fa67b132cb78b795b9d1b18dbf7728d7f545a/xpublish/rest.py#L66-L82

Could become

    def __init__(
        self,
        datasets: Optional[Dict[str, xr.Dataset]] = None,
        routers: Optional[APIRouter] = None,
        cache_kws=None,
        app_kws=None,
        plugins: Optional[Dict[str, Plugin]] = None,
    ):
        if isinstance(datasets, xr.Dataset):
            raise TypeError("xpublish.Rest no longer directly handles single datasets. Please use xpublish.SingleDatasetRest instead")

        self.setup_datasets(datasets or {})
        self.setup_plugins(plugins)
        ...

If we're mucking about with Rest.__init__ anyways, one thing that I was thinking would be nice to make a little clearer is routers=.

Since they refer to routers that apply only to datasets, not ones that are at the top level of the app, how about renaming them to dataset_routers to match up with plugins? We could even keep both kwargs for now, but throw a deprecation warning when routers= is used instead of dataset_routers=.

[jhamman]

My suggestion would be to break things in this project for a while. Usage is fairly low and development has been slow. At some point, we'll want to be careful about introducing breaking changes but I think for now, we can be fairly cavalier about these things (provided they are well reasoned).

[benbovy]

Agreed, @abkfenris' TypeError suggestion is good enough to me.