PyBaMM icon indicating copy to clipboard operation
PyBaMM copied to clipboard

Use `deprecation` package for deprecation warnings

Open brosaplanella opened this issue 3 years ago • 4 comments

Replace existing deprecation warnings/errors with deprecation package so tests fail after a few releases.

brosaplanella avatar Apr 20 '22 16:04 brosaplanella

I have been looking into this and I have a (probably very basic) question. The deprecation package uses decorators and it seems designed to deprecate entire methods directly. How should I use them when we want to deprecate either a few lines of code, or some arguments? For an example, see l84 of simulation.py.

brosaplanella avatar Apr 25 '22 13:04 brosaplanella

The deprecat package (built on top of the deprecate package) allows us to put deprecation warnings for arguments - https://github.com/mjhajharia/deprecat#deprecated-kwargs. Not sure how we can deprecate some particular lines of code.

(deprecat was built as deprecate doesn't allow deprecating arguments - https://deprecat.readthedocs.io/en/latest/source/introduction.html#some-background)

Saransh-cpp avatar Jun 03 '22 14:06 Saransh-cpp

@brosaplanella I was looking through some solutions for this and while I couldn't find anything to deprecate some lines of code, there are indeed ways for deprecating arguments: in addition to deprecat mentioned by @Saransh-cpp, there's also pyDeprecate – a similar library with many more customisations available, applicable on both classes and methods along with forwarding inputs to newer arguments.

Both of them will come at the cost of an extra dependency, so here is a purely Pythonic way which could perhaps be implemented as helper functions in pybamm/doc_utils.py? (see #2597). It transfers parameter values too but can be modified to just raise a warning on deprecation: https://gist.github.com/ZoomQuiet/f0113711181750e56ddfb51756d5c7fc

agriyakhetarpal avatar Mar 10 '23 14:03 agriyakhetarpal

pyDeprecate looks really nice. We could also implement a new helper function, but having an extra dependency doesn't sound too bad to me (if it fulfills all our requirements).

Saransh-cpp avatar Mar 14 '23 19:03 Saransh-cpp