Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add MIME registry #5765
Add MIME registry #5765
Changes from all commits
c022585
e4a128f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should work the other way around:
Crystal::System::MIME
should provide a method that returns (or yields) amime.types
file thatMIME
would try to parse.That would avoid the circular dependency that
MIME
depends onCrystal::System::MIME
which itself depends onMIME
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the circular dependency either. But the fact that
Crystal::System::MIME.load
utilizesMIME.load_mime_database
is already a platform specific implementation.It could certainly be reversed if
load
yields if it needs to load a file. On windows it just wouldn't yield, but load the database differently. But that's baking platform-specifics into the abstraction API.That's how I chose to implement it and I think it should stay this way. But I won't hurt about changing if requested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's similar with
Crystal::System::Time.load_localtime
btw., the Unix implementation calls the non-platform-specific implementationTime::Location.read_zoneinfo
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what makes me wary is that something from Crystal::System is initializing an external namespace; and ::MIME should initialize itself using Crystal::System. I wish we could find a better way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could pull the platform-dependent initialization implementations out of
Crystal::System
into the main namespace. But that doesn't change anything, really.