Replace clobber parameter with overwrite - #5171

A note on the implementation:
I've implemented this at the file.py level, in the _File class, which is used by most output routines. Exceptions are diff.py and table.py, which use their own output routines.

A note on the unit tests:
I've replaced all clobber occurrences with overwrite. Except for the newly written unit tests, there is thus no test to see if clobber actually works with the old routines.
The new unit tests are perhaps a bit awkwardly placed: I wanted to make use of the FitsTestCase temporary file utilities, but the actual tests of overwrite vs clobber seem better placed outside any test class (but that would require a different mechanism for temporary files).

I'm not really sure if it may be a problem that you added the overwrite parameter in between existing parameters. Personally I would have preffered something along the lines of func(..., overwrite=None, ..., **kwargs). Placing the overwrite at the position where the old clobber was to prevent problems with code using clobber (or subsequent parameters) as positional parameters. Especially for HDUList.writeto() I often used it as positional parameter.

Also catching clobber as kwarg will hide it from the auto-completion of runtime environments (like Notebooks) and makes it much easier to remove it some day without having to alter every function definition again. You could propagate it through the internals and check for clobber in the end.

On the other hand I'm not really sure if using **kwargs just to deprecate one argument is such a good alternative because one would need to check if any unsuitable keyword argument or typo-parameter was catched as well.

Thank you for this thorough PR. I'll have a closer look in the next days.

@MSeifert04 Thanks for catching that; I had totally overlooked the fact that keyword arguments might be used as positional arguments in existing code. I had simply placed overwrite next to clobber.
I'll wait a bit to see what other people might say, but otherwise I'll move overwrite to the far right in the function definitions. (I'm in favour of being explicit, i.e., not using **kwargs.)
Once we get to astropy version 2, we can move it back to a more suitable place, if so wanted.

I'll have a look at the Sphinx directives.

+1 for adding overwrite, but I think that deprecating clobber is very annoying for a very little gain. clobber must be used in a lot of code, and I fear people will get warnings for a long time before having a clobber-free code base. This is the kind of things that will bother people. So they will filter warnings, and all other AstropyDeprecationWarning by the way, making these warnings useless.
Also supporting older Astropy versions (like the 1.0) will require to add checks for Astropy's version to avoid the warnings.
Supporting both option is not so complicated.

Replacing clobber with overwrite actually makes the unified I/O more consistent. This has been discussed elsewhere before although not linked to this PR. I agree with @MSeifert04 that it is better to "hide" clobber in **kwargs. Maybe something like the pseudo-code below:

if clobber in kwargs:
    issue_deprecation_warning
    overwrite = clobber

Or if you want to be stricter, maybe something like (but I think the former is nicer to people who already use clobber):

if clobber in kwargs:
    issue_deprecation_warning
    if overwrite is not None:
        raise ValueError('You have to explicitly set overwrite=None to use clobber')
    overwrite = clobber

no test to see if clobber actually works with the old routines

We should at least keep one test that uses the old clobber keyword successfully and make sure it still overwrites an existing file, just because it is so widely used.

I see a few different opinions and suggestions above, and I'm not sure how now to proceed.

saimn makes the correct argument that clobber in a (py)fits related context is widely used, and deprecating (and finally removing) the keyword may cause problems for users further down the road (as such a change will take a while to propagate through dependencies).
I'm in favour of removing it eventually, and then better start issuing warnings early. I don't expect clobber to be removed before astropy version 2.
Users that still use dependencies that haven't replaced clobber by overwrite may then be forced to use astropy 1.x though, while they may want to use some astropy 2.x features in other parts of their code.

There is also the suggestion of moving clobber into kwargs. That does create a problem for cases where a positional argument is used to set clobber, unless of course overwrite is put at that place in the function arguments.

I'm not really sure how much checking there should be to see whether both overwrite and clobber are used, and if they are inconsistent. It does pose the problem that the default value has to be None instead of False in the function signature, since otherwise it's not possible to verify that only one of the keywords has been used. But perhaps it should be allowed to use both (with a DeprecationWarning), and just check for inconsistency in that case? (But this raises immedate ValueErrors with an overwrite=False default while a user uses clobber=True.)
Related to that: as per MSeifert04's note, should we support anything truthy or falsely, or should be enforce strict boolean values? My preference is the latter.

I propose:

1. Move clobber to kwargs and put overwrite in its place, as you suggested, to support positional argument call signature.
2. Never set clobber nor overwrite to None (IMHO, that just complicates the logic). If clobber is present, always just clobber but issue a DeprecationWarning and ignore overwrite. This is to satisfy backward compatibility until it is removed, without raising any error.

Feel free to discuss further. Those are just my personal thoughts.

I agree with @pllim but I want to propose another point:

• Put the logic how to resolve the clobber/overwrite parameters in a seperate function. (Replace clobber parameter with overwrite #5171 (comment)) That way it is garantueed that each high/low level functionality behaves exactly the same. It makes it much easier to edit, move or expand it if the need arises.

@pllim - I think the 2nd part of your point 2 is not backward compatibility, but a possibility to cause a big headache when the actual removal of clobber happens.

Currently one gets a big TypeError when specifying both clobber and overwrite, so I'm 👎 on changing that to a DeprecationWarning while accepting it. If anyone takes the effort to change their legacy code to have overwrite, they may remove clobber right away.

So I would suggest to raise the DeprecationWarning whenever clobber is used, but keep throwing errors on the user if they try to use both.

Another way but be using a decorator to catch the parameters passed to the functions (this would work best at the high-level-functions) and resolve the conflicts before it is actually executed. Something (very roughly) like this:

import functools

def deprecateArgument(position):
    def outerwrapper(function):
        # position is the position (number) of the overwrite parameter
        # in the function definition. This could also be extracted with
        # inspect.getargspec or inspect.Signature.
        # position = list(inspect.signature(function).parameters.keys()).index('overwrite')
        @functools.wraps(function)
        def wrapper(*args, **kwargs):
            # The only way to have clobber inside the function is
            # that it is passed as kwarg because the clobber parameter was
            # renamed to overwrite
            if 'clobber' in kwargs:
                clobber = kwargs.pop('clobber')
                print('clobber is deprecated.')

                # Check if an overwrite was given
                overwrite_in_args = len(args) > position
                overwrite_in_kwargs = 'overwrite' in kwargs

                # Annoy the user if he annoys us. They shouldn't have
                # specified overwrite and clobber!
                if overwrite_in_args or overwrite_in_kwargs:
                    raise TypeError('cannot specify overwrite and clobber.')
                # Otherwise use clobber as overwrite
                kwargs['overwrite'] = clobber

            return function(*args, **kwargs)
        return wrapper
    return outerwrapper

@deprecateArgument(1)
def test(a, overwrite=False, c=10):
    print(overwrite)

@bsipocz , whatever is fine. I'll let the main package maintainer decide which idea to adopt.

I've now implemented the approach suggested by @MSeifert04 (thanks!), using a decorator. I made the decorator slightly more generic: it takes two arguments, the old and new keyword. See the diff for astropy.io.fits.util in this PR for the current implementation.
That makes me wonder if it's worth writing a more generic decorator for replacing keywords in functions & methods, and add it to astropy.utils.decorators instead. That module already contains decorators to mark a function, class or attribute as deprecated (though in this case, it's more a keyword replacement).

Documentation-wise, I've left clobber in the parameter section of the doc-string, with the suggested ..deprecated directive (and ..versionadded for overwrite). I haven't build the docs, so I don't know what it looks like. Perhaps the deprecation note is better placed at the end of the doc-string (outside the parameter list), and clobber completely removed from the parameter documentation (since the function definition doesn't contain it either).
Using ..versionchanged at the end is another option (with no clobber in the parameter list, and no ..versionadded to the overwrite description):

.. versionchanged: 1.3
   overwrite replaces the clobber keyword. Clobber is deprecated.

An example in the Python or numpy documentation would be good to use, but I haven't found one for this particular situation.

It is approved, so merging. Thanks, @evertrol and @MSeifert04 !

p.s. "I am in fits and clobbered by trying to clobber a FITS file" does not sound that good by replacing "clobber" with "overwrite".

👍 from me too (if a little late 😉 )