Remove mention for clobber from all FITS functions - #6381

Hi there @drdavella 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labelled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

Wow, you are ahead of the game. 😅

Whoops, I guess that's what the last-before-release tag means?

I'm not sure this really has to be last before release? (unlike the Python 2 removal which we do want to delay)

I am a bit traumatized by the backlash of just deprecating it for 1.3, now we are talking about complete removal. Anyway, feel free to remove the tag if you feel otherwise.

@drdavella - last-before-release means that those will be merged very close to the the next release to give a bit more time for affiliates to update their usage and also their CI systems.

While we may not want to wait with this until the very last moment, I suspect it won't get merged much before the python2 removal.

Maybe a new label called "wait a little bit but not last minute"? 😅

@astrofrog - I think it's still a good idea to give it a bit more time. Deprecationwarnings are already been issued and should fail their CIs (assuming they have opted in, but not everyone does), but probably there are still packages out there that use clobber.

Didn't mean to stir anything up here; I've just been looking for bite-sized FITS-related issues to get my feet wet with astropy development. Feel free to hold off on a merge for as long as you want.

@drdavella - Not at all, thank you very much for working on io.fits. I'm sure most of us can't wait for the moment to hit merge on these and remove all the deprecated stuff including python2 :)

The directives (.. versionadded) should be kept. I don't see any reason to remove them.

re: the .. versionchanged information in the docstrings: maybe it shouldn't be removed, but it seems like it should at least be updated to reflect more current information?

at least be updated to reflect more current information

Seems reasonable. Is this too verbose?

.. versionchanged:: 1.3
         ``overwrite`` replaces the deprecated ``clobber`` argument.

.. versionchanged:: 3.0
         ``clobber`` is no longer supported; Use ``overwrite``.

What about:

.. versionadded:: 1.3
    ``overwrite`` replaces the ``clobber`` argument (deprecated in 1.3; removed in 3.0).

On reflection it probably should've been a versionadded directive not a versionchanged because the overwrite was added. But "changed" makes also sense because it was more a "renaming" than an addition...

But some kind of directive should be kept, just so people investigating a failure using the docs find a note about clobber (and that it was replaced by overwrite) without having to dig through the changelog.

But some kind of directive should be kept, just so people investigating a failure using the docs find a note about clobber (and that it was replaced by overwrite) without having to dig through the changelog.

Would it make sense to have some kind of decorator equivalent to the deprecated one but for actual deleted arguments?

@astrofrog Just to modify the documentation? That doesn't sound very robust (I actually needed to remove the doc-changing part of deprecated_renamed_argument because it was too fragile) and a bit overkill. Why not just leave the directive in the docs? Python/NumPy also keep them around "forever" (at least for several years).

Looks good for me in terms of code, but I think this change should be delayed for at least one version (and probably more). The deprecated warning was just added in 2.0, and I already encountered a few codes that emit it. clobber is widely used, and I don't think it will be fixed everywhere before 3.0. Also it's a pain to support older Astropy versions with this change, so really I think we need to give people more time.

I feel that since there will be big changes in 3.0, it's OK to get this in there, too. However if we decide to delay it further, then it would be nice to create a new milestone for the decided version making sure this doesn't slip through.

I think we can merge this soon before coordination meeting, unless someone objects? @MSeifert04 and @saimn , is the code still okay to merge?

Code-wise this has been ready for a while. But I would've preferred to keep it until 3.1 or 4.0 because it's really used a lot in scripts/downstream packages and there is simply no pressing need to remove it as soon as possible... and one minor release is just short for a deprecation.

Okay, maybe that came off too strong. I would like to have a longer deprecation period but I don't care too much.

As explained above I think this should wait for at least one major version. There is no urgency here and the amount of code involved (in Astropy) is small, so I don't see any issue to keep this longer. Give people time, clobber is everywhere 😉

Should we re-milestone then?

If it's ok for you and @bsipocz 😌 . Which milestone, 3.1 (next major version) or 4.0 (next LTS) ? And these milestones should be created.

isn't it "major.minor.micro"? 😄

Are we doing semantic versioning ? (I was referring to this roadmap: https://github.com/astropy/astropy-APEs/blob/master/APE10.rst#roadmap)

We're not doing strict semantic versioning (https://github.com/astropy/astropy-APEs/blob/master/APE2.rst), so next major (or feature) release will be 3.1, then 3.2, 3.3 and 4.0.

The 3.0 is super major in term of being an exception of not being an LTS, but tons of code will be removed (thus we expect some roughness, that warrant it to be a 3.0, and also not being LTS).

Also, I'm happy to create a 3.1 milestone, but would rather avoid doing a 4.0 and tip-toe around PRs that are planned to be opened for another 2 years. Although the appendum hasn't been formally merged (astropy/astropy-APEs#20), there was an agreement (or seemed to be one), that deprecations should not be too long. People are given the LTS option, they should really make up there mind whether to use the cutting edge or the super stable version.
I agree that "clobber" may be the an exception, and may benefit from a long deprecation, but also have my suspicion (admittedly without much proof) that most of the code that still uses it will break anyway with 3.0.

I'd have to say I was with @saimn on this originally as there is a lot of legacy code that uses clobber and not everyone has time to maintain their code. But the argument about 2.x -> 3.x by @bsipocz kind of wins me over. Since the next version is going to break a lot of codes as it is, there would be sense to have users only get mad at us with one update rather than twice. But it would be good to further discuss this next week.

@eteq @adrn @taldcroft and I discussed this further at the CC meeting in NY today and we think that deprecating things is fine but we should only remove them if it gets in the way of future development. A LOT of people use clobber in existing code and we will annoy a lot of users if we merge this. So we think this should not be merged for the foreseeable future, potentially ever.

So if others agree with our assessment, this PR should be updated to just include the documentation fixes (basically clobber shouldn't be mentioned in any documentation)

for the record: I was also there, and see the logic behind the decision.

I just rebased and updated with documentation-only changes. I don't think this needs a changelog update any more.

If people are going to switch to v3 without PY2 support, lack of clobber wouldn't be their number one concern, but since there is a consensus, I have updated the text in this PR to close the issue, as this is the last remaining item listed and looks like clobber has clobbered our attempt to clobber it out forever.

Good news, and thanks @drdavella for the quick update.

I've cancelled all but docs builds.

Thanks @drdavella!