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

Deprecate compound input fn in favor of Reader #1327

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

HeroicKatora
Copy link
Member

@HeroicKatora HeroicKatora commented Oct 5, 2020

Any comments on the questions below are welcome.

In light of #1318 and previously #1291 , #1232 , #1231 , #1072 , #792 (possibly more), I conclude that opening files with the functions in the main module is at least misunderstood but quite possibly a bad interface in itself. It's often not clear how to do combine aspects of these methods by performing parts of them manually—such as how to open a file with a known image type. In that sense these functions are too numerable and too specific at the same time.

io::Reader intends to fix this by encapsulating the known inputs as an object and then presenting builder-style methods for adjusting the details, independent of whether one opens a file or reads a slice of data from memory.

This PR intends to deprecate the utility wrappers in dynimage, not the primitive methods in io/free_functions.rsguess_format and load. This was always planned.

The main concern is to find the sweet spot for the scope of the deprecation.

  • Are we okay with the set of methods being deprecated?
  • Are the replacements actually easy enough to be discovered? Should we add some more documentation concerning this? One particular sore spot might be loading from memory since no method in Reader takes a byte slice as input. For new Rusteceans the generics might be unecessarily confusing.
  • Should we delay this until a writer has been added?

@HeroicKatora HeroicKatora added discussion kind: documentation An enhancement that does not change function easy good first issue labels Oct 5, 2020
@fintelia
Copy link
Contributor

fintelia commented Oct 6, 2020

I'm very much in favor of deprecating these functions, but I also think we should rename io::Reader at the same time since it would be much harder to do later. (We should of course keep the old name as an alias for a while to have backwards compatibility).

My concern with the current name is that it is too generic. It only barely manages to avoid conflicting with the Read and BufReader names in stdlib, and the module scope doesn't disambiguate because the standard library also has an io module. Worse people commonly use std::io; so that they can disambiguate std::io::Error from other errors so they then can't also use image::io.

My preference would be to switch to calling it ImageReader and making the io module internal to the crate.

@HeroicKatora
Copy link
Member Author

HeroicKatora commented Oct 6, 2020

I don't share that sentiment of name conflicts outside the library at all. Namespaces exist to avoid that exact problem of complicating the choice for names by collisions. True glob-imports are discouraged in any case. You're free to use any of:

use image::io as imio;
use image::io::Reader as ImageReader;

@HeroicKatora
Copy link
Member Author

Perhaps the examples should use this renaming import though. That would be easier to copy&paste which is part of the motivation for having examples in the first place.

@fintelia
Copy link
Contributor

fintelia commented Oct 6, 2020

Using renaming imports would be an improvement, but I want to think through a bit more. I guess my concerns are:

  1. Discoverability: The generated docs are too cluttered, but despite being one of the most important types in the crate, io::Reader is particularly hidden.
  2. Code readability: Seeing Reader::new(...).decode() tells you almost nothing about what it being read. Even the io prefix doesn't make things more clear. Your suggestion of using renaming imports largely resolves this point.
  3. Consistency: Other names like ImageDecoder include the Image prefix in them. Probably the least important item.

@ripytide
Copy link
Member

ripytide commented May 26, 2024

I agree with renaming Reader -> ImageReader as it a more explicit name, the io module also only contains three structs at the moment so I don't feel that constitutes a public module so I'd support making that private too. In fact, I feel the same way about image::math which only contains the Rect type at the moment, that could easily be exported at the top-level.

I've opened #2243 to implement this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion easy good first issue kind: documentation An enhancement that does not change function
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants