-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Export MediaType from http_parser #2205
Conversation
What would be the use case here? Can you give an example? I'm tempted to decline this PR, since exporting other libraries is rather bad form. Especially if it's just exporting one class of it. If you need http_parser code, you should depend on it. For dio, it's pretty much an implementation detail, and exporting it makes it no longer an implementation detail, and thus harder for us to evolve related things. |
Yea, I don't think we use this class as parameter anywhere. If you want to use this for parsing mime/media types you should import this in your package. |
Well, we accept dio/dio/lib/src/multipart_file.dart Line 45 in 87e6b1d
But I agree that users should import the package explicitly. |
I 100% disagree. Why should users have to add a dependency which is an implementation detail of dio just to use the API? Exporting types from other packages is a very normal thing to do and is common in popular packages. |
I'm trying to find best practices for re-exporting packages from the package but failed to find something. Given that we have previous arguments about "best practice", I'm reopening this for further discussions. |
Well, let's do it. |
Should we document somewhere that we're re-exporting these classes, in case people will run into name conflicts? Is it even possible to run into problems? |
What if we use a type alias with a Dio prefix and re-export that? It will still accept the original MediaType type so it should not break anything. |
That would be a great idea. @Reprevise Could you make that attempt? |
I could do: typedef DioMediaType = MediaType; Though I prefer the exporting approach so that users would just get an |
They should not get any warning, the |
Not saying they should get a warning, but I just mean if they want to get rid of the http_parser import and the explicit dependency of it in pubspec.yaml. |
Those who care about removing the explicit dependency are free to change from |
I have updated to use the type alias. The reason I had the constructors take |
For me it looks good, can you maybe add a test which ensure that we can still pass the |
Co-authored-by: Alex Li <[email protected]> Signed-off-by: Benjamin <[email protected]>
@Reprevise Could you solve the format issue? |
What format issue? |
https://github.com/cfug/dio/actions/runs/9175832587/job/25229693166?pr=2205 |
Code Coverage Report: Only Changed Files listed
Minimum allowed coverage is |
Resolves #2212
New Pull Request Checklist
main
branch to avoid conflicts (via merge from master or rebase)CHANGELOG.md
in the corresponding packageAdditional context and info (if any)
This PR exports
MediaType
from the http_parser package so that way users don't have to import a (probably) transitive dependency.