Skip to content
This repository has been archived by the owner on Nov 19, 2024. It is now read-only.

Change Identify API to accept only an io.Reader #322

Merged
merged 5 commits into from
Mar 17, 2022
Merged

Change Identify API to accept only an io.Reader #322

merged 5 commits into from
Mar 17, 2022

Conversation

jhwz
Copy link
Contributor

@jhwz jhwz commented Feb 19, 2022

Implements a "headerReader" which caches all reads, and returns an io.MultiReader so we can return a reader which will read from the same point in the input stream.

This is particularly useful for the Identify function, which can now just accept an io.Reader and not require an io.Seeker which is somewhat unnecessary. Also, it will also always return the Reader despite the error which the caller can then read from as normal.

On reflection it may be useful to still support io.Seeker's in another function, maybe these changes should be moved somewhere else. It is more useful for files because you can still continue to use the *os.File instead of the more restrictive io.Reader.

Also includes some tests for the mentioned functions.

This PR is based somewhat off the discussion in #317. I like this package and want to shift some of my projects that have a less clear API to using this. As an alternative, maybe if we expose the formats map, I can just implement this in a separate project.

@mholt
Copy link
Owner

mholt commented Mar 1, 2022

Thanks for this PR, I've been swamped catching up on things after a bout of sickness, but I like where this is going and I'll try to review it soon!

(A breaking change is OK since we're still in development, pre-release.)

@mholt mholt changed the title [breaking] Change Identify API to accept only an io.Reader Change Identify API to accept only an io.Reader Mar 1, 2022
@mholt
Copy link
Owner

mholt commented Mar 17, 2022

@jhwz Thanks for this change. I took the liberty of refactoring a bit and renaming things to clean it up and make it more consistent with the existing code style. (I also moved the new code into formats.go and formats_test.go instead of their own separate files.) I also fixed a minor bug in the tests. I appreciate the thought that went into your change; I also think we don't have to do as much of the buffer management ourselves though, we can use existing types in the standard library, more like how I did it in Caddy's Layer 4 module: https://github.com/mholt/caddy-l4/blob/56bd7700d889f2ffd52353c241819ddbc7745ff6/layer4/connection.go#L72-L108

I think it'll be easier to maintain and is a little easier to read, and less error-prone.

Anyway, thank you for the contribution! I'm going to resolve the merge conflict and then finish this.

@jhwz
Copy link
Contributor Author

jhwz commented Mar 17, 2022

@mholt Just flicked through your changes - made a lot of sense. Thanks for merging - the new API will be much more convenient for my use cases!

Copy link
Owner

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Thanks again for the contribution. Let me know if you have any questions or feedback! 👍

@mholt mholt merged commit 4fc750e into mholt:master Mar 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants