Skip to content
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

ImageSlice docstring needs updating #1355

Closed
jni opened this issue Jun 13, 2020 · 6 comments
Closed

ImageSlice docstring needs updating #1355

jni opened this issue Jun 13, 2020 · 6 comments
Labels
documentation Documentation content and tooling
Milestone

Comments

@jni
Copy link
Member

jni commented Jun 13, 2020

📚 Documentation

See discussion here:

#1343 (comment)

Specifically, I object to using the type alias ImageConverter, and I strongly object to using it in the docstring. Additionally, the description of the argument should explain that any array -> array callable will work here.

@sofroniewn sofroniewn added the documentation Documentation content and tooling label Jun 13, 2020
@sofroniewn sofroniewn added this to the 0.3.5 milestone Jun 13, 2020
@pwinston
Copy link
Contributor

pwinston commented Jun 15, 2020

I think there are two separate questions here:

  1. When should a type alias be used vs. just spelling out the full type everytime.
  2. When using a type alias, what should the docstring say: the alias, the full type, or both.

For 1 my feeling is the codebase is nothing but a huge pile of symbols: variables, arguments, parameters, methods, functions, classes, modules, packages. To me introducing nicely-named symbols for types is just part of programming. And not doing so leads to repetitive bloated code that's hard to read and maintain. That said, I know some reasonable people feel the exact opposite, they feel that type aliases obscure things.

For 2 the idea that the type should be different in both places seems crazy to me, as in you are telling the developer two different answers the same question. But the idea that it should be repeated identically in both places seems crazy too, as in it's pure duplication. The whole idea of tediously entering types into comment blocks which might get totally out of sync with the code seems like a bad idea. It must be vestigial from before type annotations existed? I think the numpydoc world is heading towards using type annotations as the single point of truth, but they have not made the transition yet:

In both cases I'm happy to do whatever the project wants here. I think projects know their own unique needs and goals. However ideally we can set clear guidelines so in the future people can get the PR right the first time without as much discussion.

@pwinston
Copy link
Contributor

pwinston commented Jun 15, 2020

I think there's just two concrete decisions to make for this PR:

  1. Leave ImageConverter in the code or replace it with Callable[[ArrayLike], ArrayLike] in the two places it appears?
  2. Leave ImageConverter in the docstring or put Callable[[ArrayLike], ArrayLike] there instead?

And then decide in general when do we use type aliases and how do we document them?

@sofroniewn sofroniewn modified the milestones: 0.3.5, 0.3.6 Jun 16, 2020
@sofroniewn sofroniewn modified the milestones: 0.3.6, 0.3.7 Jul 14, 2020
@jni
Copy link
Member Author

jni commented Aug 21, 2020

Finally revisiting this during my review of #1565. I would like to replace it with Callable[[ArrayLike], ArrayLike] wherever it appears. It is longer but instantly readable to anyone with a passing familiarity with NumPy and type annotations.

@pwinston
Copy link
Contributor

pwinston commented Aug 21, 2020

I got rid of the ImageConverter type alias just now in #1565. However what does the style guide say about when to spell out a type, when to use a type alias, and when to create a user-defined type (class)? You want to give people a fighting chance of doing it right the first time, not make it into a "ask Juan" type of thing. For instance we do have type aliases in places.

@jni
Copy link
Member Author

jni commented Aug 21, 2020

We don't have a specific style guide. However the Zen of Python (import this) is the overarching guide, and it states "explicit is better than implicit". (It also says "practicality beats purity". So some type definitions might be judged as necessary. In this case I don't think it was — it's a judgement call. And I acknowledge that! It just happens to be one that I feel strongly about.)

@jni
Copy link
Member Author

jni commented Aug 22, 2020

Fixed in #1565. Thanks @pwinston!

@jni jni closed this as completed Aug 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation content and tooling
Projects
None yet
Development

No branches or pull requests

3 participants