Handle chararray deprecation - #19267

I think this is okay but it does require #19233

Oh, sorry, I didn't see this before I commented below.

I'd tend to just import from np.char import array as get_chararray inside a warnings filter above.

+1
Both approach seem fine to me, but I'd rather we avoid mixing them.

@mhvk - do you mean to use that for a simplified version in 7.2.x ? (which seems reasonable to me but will be in a follow up PR).

I realize now that what I wrote was confusing, since of course the numpy version of get_chararray returns the numpy chararray rather than our new one that shows deprecation warnings. In that sense, my comment is moot. But, arguably still a bit cleaner -- since we are deprecating -- might be to use np.char.array and then just view the result as our class. Since I wasn't absolutely sure this would not give warnings, I just tried, and it seemed to work -- I submitted it as a PR to your branch (see saimn#10).

Note that I also went ahead there and moved the deprecated_attributes inside the class; I think that is a bit cleaner (but it is a separate commit, so if you don't like it, just omit it).

One additional comment here: if in the future rstrip is applied on the io.fits ndarray column it means that trailing spaces will always be stripped for Table. So strip_spaces will become useless and default behavior will change. But I'm not sure if there a good way to warn people about this upcoming change without adding a noisy and annoying warning...

what do you mean "noisy" ? Deprecation warnings are still only visible to direct callers and warnings in general only show up once per session by default.

The warning would need to be emitted by default (since strip_spaces is False by default) and only way to remove it would be to set strip_spaces=True (option that would disappear in the next major version).

I don't think I understand your initial comment in this thread actually, let's get back to it;

if in the future rstrip is applied on the io.fits ndarray column

why would this happen ? do you expect it to be outside our control ?

Currently (and on the contrary to io.fits which relies on chararray to do so), Table.read does not strip trailing spaces by default and strip_spaces was added no long ago to offer this option. If in the future rstrip is enforced in io.fits (calling it explicitly instead of relying on chararray) then this would propagate to Table.read and the default behavior would change.
My point above is I don't think there is a good way to warn about this upcoming change. I think it's fine for a major release, but I'm mentioning it because this will likely break some code in the wild (e.g. people using t[t['somecol'] == 'SOMEKEY '] to select some rows).

I see. If I got this right, there is actually a way to follow a proper deprecation cycle here, and preserve strict backward compatibility for those who want it.

from typing import Literal
UNSET = object()

def foo(*, strip_spaces: bool | Literal[UNSET] = UNSET) -> None:
    if strip_spaces is UNSET:
        warn(
            "default is currently False but will change to True in a future version, "
            "pass the argument explicitly to silence this warning.",
            AstropyDeprecationWarning,
            stacklevel=2,
        )
        strip_spaces = False
    ...

(note that we could use None as a sentinel value but I don't like it because it behaves too much like False where a bool is expected)
This of course assumes that we could maintain the current default behavior (strip_spaces=False) as an opt-in. Is this possible ?

The problem is that this is pretty noisy. Though I guess one could at least only warn if the stripping actually has an effect.

We could use a different default value indeed, this would force people to adapt their code and use strip_spaces=True, but then this option would disappear in the next major release (and probably remain forever in people's code)...

Maintaining the strip_spaces=False option would be difficult since the stripping would be done pretty deep in io.fits. Stripping trailing spaces should be the default and only behavior, it was not done in Table.read for historical reason but at some point this change need to happen and dropping chararray is the good occasion to do it. The question is whether we want to be noisy about it or just make the change at some point with the risk of breaking some code. Anyways people that rely on the old behavior (e.g. t[t['somecol'] == 'SOMEKEY ']) will need to change their code, and there is no way to know who relies on this. We can detect if some spaces are stripped but that does not mean that people's code rely on it.

ok if we cannot maintain strict backwards compat then I agree my proposal shouldn't be implemented and there is probably no way to warn users ahead of time programmatically. I think the best thing we could do at this time is go with your approach on main and actively seek for feedback in downstream projects (we probably should do the probing ourselves where we already know astropy.io.fits is heavily used).

I realize now that what I wrote was confusing, since of course the numpy version of get_chararray returns the numpy chararray rather than our new one that shows deprecation warnings. In that sense, my comment is moot. But, arguably still a bit cleaner -- since we are deprecating -- might be to use np.char.array and then just view the result as our class. Since I wasn't absolutely sure this would not give warnings, I just tried, and it seemed to work -- I submitted it as a PR to your branch (see saimn#10).

Note that I also went ahead there and moved the deprecated_attributes inside the class; I think that is a bit cleaner (but it is a separate commit, so if you don't like it, just omit it).