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

[pkg-mime] contenttype to ext #431

Merged
merged 38 commits into from
Sep 11, 2024
Merged

Conversation

mx1up
Copy link
Contributor

@mx1up mx1up commented Aug 26, 2024

this is a PR moved from dart-lang/mime repo

I refiled a PR here as requested. I hope this PR can finally be merged now...


based off pr dart-archive/mime#15 , afterwards I realized in the meantime mimeFromExtension had already been added 🙄 but without defaults. I went ahead anyway since @kevmoo https://github.com/dart-lang/tools/issues/397PR's are still welcome.
So this PR mainly adds the defaults and custom error handling (orElse and nullable version), while maintaining backwards compatibility.

example default:
image/jpeg now nicely maps to "jpg" instead of "jpe"

I copied firstWhereOrNull from the collection package to avoid a dependency.

fixes #411
fixes #397

Edit: removed collection dependency

mx1up and others added 30 commits August 26, 2024 19:18
…d extension not. for backwards compatibility: add `extensionFromMimeOrNull`, and add an `orElse` param for `extensionFromMime` as the standard behavior results in creating an invalid filename.
…ith multiple extensions. There are quite some, but most of them are legacy, and new mimetypes tend to map to 1 extension (no 3 chars limit anymore), so less useful in the future
@dart-lang dart-lang deleted a comment from github-actions bot Aug 28, 2024
@mosuem
Copy link
Member

mosuem commented Aug 28, 2024

This seems to need a major version rev as extensionFromMime was removed. Also dart format is complaining.

@mx1up
Copy link
Contributor Author

mx1up commented Aug 28, 2024

This seems to need a major version rev as extensionFromMime was removed. Also dart format is complaining.

@mosuem thx, now I remember dart-archive/mime#81 (comment)

Is it ok though to introduce a major bump, just for this? I mean, is it ok to remove the method, we can as well leave it in and have a non-breaking release which is always better, i guess? better to mark it deprecated?

@mosuem
Copy link
Member

mosuem commented Aug 29, 2024

In my view a major version release is fine, this is why we have semantic versioning.

@mx1up
Copy link
Contributor Author

mx1up commented Aug 29, 2024

@mosuem ok, I made a major bump. I thought I'd read some breaking change policy that we'd strive to non-breaking changes as much as possible but I guess it does not apply here 👍

I also fixed the test and added some more default extensions along the way. I'll check the pipeline for any more errors when it runs.

@mosuem
Copy link
Member

mosuem commented Aug 30, 2024

Yes and no - no breaking is the better option if possible, as it makes the fixes and features available to users with restraints of the older version. But this shouldn't keep us from evolving the API if we feel it is needed.

@mosuem
Copy link
Member

mosuem commented Aug 30, 2024

It seems that package:test has a dependency on version 1.0.0. So either we don't rev the version here after all, just marking extensionFromMime as deprecated, or loosen the dependency in package:test after checking that extensionFromMime is not used there.

@mx1up
Copy link
Contributor Author

mx1up commented Aug 30, 2024

@mosuem I realized it is not a function that has been deprecated, but the return value that has become nullable. So I guess making a breaking release is the only option (@lrhn already mentioned we don't want to introduce another method like extensionFromMimeOrNull for example).

package:test itself does not depend on package:mime, it depends through package:shelf_static. I checked the source of shelf_static and did not find any usages of extensionFromMime. So it seems safe to loosen the constraint there to >=1.0.0 <3.0.0.

Is my reasoning correct? I basically have to create a PR against package:shelf_static.

@mosuem
Copy link
Member

mosuem commented Sep 2, 2024

Is my reasoning correct? I basically have to create a PR against package:shelf_static.

SGTM!

mx1up added a commit to mx1up/shelf that referenced this pull request Sep 2, 2024
mx1up added a commit to mx1up/shelf that referenced this pull request Sep 2, 2024
@mx1up
Copy link
Contributor Author

mx1up commented Sep 5, 2024

@mosuem i made the PR a few days ago, but I guess it will take some time. Unless maybe you can review there too ;) (or maybe at least trigger a pipeline, I did not manage to run tests locally)

natebosch pushed a commit to dart-lang/shelf that referenced this pull request Sep 5, 2024
See dart-lang/tools#431 (comment)

An upcoming breaking change does not impact usage in this package.
@mx1up
Copy link
Contributor Author

mx1up commented Sep 6, 2024

@mosuem shelf PR has been merged and released. Can you rerun the pipeline please?

@kevmoo
Copy link
Member

kevmoo commented Sep 6, 2024

I hit the button to rerun failed checks

@mx1up
Copy link
Contributor Author

mx1up commented Sep 6, 2024

thanks @kevmoo . it may be me, because I'm not very familiar with github actions, but I fail to find the rerun of the health checks. Dart CI build job did rerun and seems fine now 🥳

@kevmoo
Copy link
Member

kevmoo commented Sep 6, 2024

image

You might not have the keys to rerun since you're not a member of the repo...

@mx1up
Copy link
Contributor Author

mx1up commented Sep 9, 2024

@kevmoo thanks, but I meant, it seems the Dart CI jobs have been rerun, but the health checks do not seem to have been rerun (see timestamps in tooltip popups):

Dart CI

image

Health checks

image image

@mx1up
Copy link
Contributor Author

mx1up commented Sep 11, 2024

@mosuem can you rerun health checks please? they don't seem to have triggered the last time (see previous post)

@mosuem
Copy link
Member

mosuem commented Sep 11, 2024

@mosuem can you rerun health checks please? they don't seem to have triggered the last time (see previous post)

Done, but you might need to push an (empty) commit to get the newest hash. Not sure which hash a rerun will take.

@mx1up
Copy link
Contributor Author

mx1up commented Sep 11, 2024

@mosuem it seems to have worked, everything's green now! 🎉 ready to merge now? 😊

@mosuem mosuem merged commit 56282a4 into dart-lang:main Sep 11, 2024
14 checks passed
@mosuem
Copy link
Member

mosuem commented Sep 11, 2024

Done! Thanks a lot for the changes and the patience :)

@mx1up
Copy link
Contributor Author

mx1up commented Sep 11, 2024

@mosuem thanks for helping this move along! Without you it wouldn't have happened ;)

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.

ExtensionFromMime returns jpe for image/jpeg Add content type-to-extension conversion
4 participants