-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Remove markdown, use markd shard in compiler #4955
Conversation
@RX14 thank you for submitting this and getting the conversation started. A few points I noticed:
Cheers. |
return | ||
end | ||
end | ||
|
||
# Check Type#method(...) or Type or #method(...) | ||
# https://www.youtube.com/watch?v=SoIfcbsV13w |
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.
srsly
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.
Oops, that was a joke and i intended to remove it before I pushed.
But the code below probably does need some refactoring...
3f49d62
to
fd6cbfa
Compare
@luislavena Most of the conversation around whether or not to do this is in #4613, it's be best to keep the discussion there. It's listed in dependencies because the crystal compiler is actually part of the stdlib and can be required from any crystal code. It's needed at compile time, not just for specs so it's a full dependency. |
fd6cbfa
to
20a6d9a
Compare
I don't think "Improve Markdown implementation" is a suitable title to discuss what's happening here. Adding a random external dependency to the compiler has so many obvious downsides that I didn't think it would need to be said. It gives too much control to the owner of that shard. (At the very least move it to crystal-lang org and give the original author non-force push access) It greatly complicates packaging and installation. Obviously a circular dependency of Crystal -> Markd -> Shards -> Crystal is very bad (actually I don't quite see how to even resolve it, since Shards is not even a dependency of Crystal). But this also makes Crystal extremely inaccessible for distribution in many forms, I'm mainly thinking of Linux distributions right now, for example, they would have to make an unprecedented move of adding a Crystal library to the distro, AND introduce a circular dependency of Crystal <-> Markd. Enterprise users would not be happy about such a dependency either. And the mixed licenses wouldn't help. |
... If you're gonna be vendoring the dependency, you must do it properly, e.g. by copying the source code or directly downloading it. |
Markdown doesn't belong in the stdlib because it's complex and has multiple competing standards. For this reason, it's much better off as a shard.
I approve the idea but can't approve embedding of shard that owned not by crystal-lang community. |
Sorry reply late, i'm glad |
@icyleaf I would really like to include your code in the standard library, but icyleaf/markd#7 made the performance terrible so now that's not possible. We should fix the performance problems and then include it in the std. |
Actually, it's icyleaf/markd#5 that made everything slower |
@asterite May you move |
@akzhan No, because to compile crystal we will then needs shards, which was said to be very difficult. |
The immediate performance hit was easily resolved (icyleaf/markd#8). And there are probably plenty of opportunities to improve performance significantly. So far, I have only worked on some of the most outstanding issues with code quality. There are still lot's of Javascriptisms in the code - it needs to be crystalized ;) Currently, performance of markd is about 9 times slower than stdlib implementation (based on markd's |
This is a stale PR at best |
No description provided.