Skip to content

DOC: Extend release notes for #26611 #26737

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 20, 2024

Conversation

mtsokol
Copy link
Member

@mtsokol mtsokol commented Jun 18, 2024

Hi @seberg,

Addresses #26611 (comment).

@mtsokol mtsokol self-assigned this Jun 18, 2024
Copy link
Member

@seberg seberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks the change looks good and thanks a lot of the quick follow-up. Sorry for not commenting earlier there.

I'll leave it open in case others want to dicuss (@mhvk might have a thought on printing).

I like the thought of adding a new mechanism for overriding that is part of printoptions, it just seems to make sense to me, although I don't know if anyone uses it :).

Thinking a bit about it now: I wonder if the old version of stringfunc(array, repr=False/True) wasn't better, though? Passing repr avoids the possible wish to add another option for str later at very little cost.

(At which point the name might be a question "override" is not a bad term, maybe even override_func, although also fine to to go with str or repr and just have the kwarg anyway.)

@mhvk
Copy link
Contributor

mhvk commented Jun 18, 2024

Thanks for the ping, I had indeed missed that the previous PR did not only remove set_string_function (good) but also introduced an alternative. I don't have a strong opinion, but it does seem to make sense to allow one to override both repr and str. @seberg - are you suggesting that the user-defined function should not just take the array, but also the repr argument? My sense is that it is easier to just provide two arguments, i.e., add add override_str as well (or perhaps in analogy with the old functions, call it str_function and repr_function).

@seberg
Copy link
Member

seberg commented Jun 18, 2024

Yeah, that was a thought, just to add fewer arguments. But I don't mind the approach of having two kwargs, either (in which case, there is also no rush).
I thought they might often come together, but if not, then passing repr=True/False might just complicate things anyway (because making the one you don't want to override do the default thing is a bit annoying).

@ngoldbaum
Copy link
Member

Ping @jni @GenevieveBuckley, I noticed the trouble you had in napari/napari#6776, this should help you get back the shorter repr you had before.

@charris charris merged commit 7687245 into numpy:main Jun 20, 2024
5 checks passed
@charris
Copy link
Member

charris commented Jun 20, 2024

Thanks Mateusz.

I went ahead and merged this because the implementation is already in and the discussion seems to be mostly about alternatives.

@mtsokol mtsokol deleted the tweak-26611-release-note branch June 21, 2024 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants