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

Remove markdown, use markd shard in compiler #4955

Closed
wants to merge 3 commits into from

Conversation

RX14
Copy link
Contributor

@RX14 RX14 commented Sep 11, 2017

No description provided.

@luislavena
Copy link
Contributor

@RX14 thank you for submitting this and getting the conversation started.

A few points I noticed:

  • Stability: Markd has no public release (yet)
  • Performance: own readme indicates is way slower than Crystal built-in version, perhaps worth some peer-review for performance bottlenecks?
  • Perhaps this should be listed as development dependencies instead, in case someone decides to use Crystal as a shard and end having to depend on Markd too.

Cheers.

return
end
end

# Check Type#method(...) or Type or #method(...)
# https://www.youtube.com/watch?v=SoIfcbsV13w
Copy link
Member

Choose a reason for hiding this comment

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

srsly

Copy link
Contributor Author

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...

@RX14
Copy link
Contributor Author

RX14 commented Sep 11, 2017

@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.

@oprypin
Copy link
Member

oprypin commented Sep 11, 2017

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.

@oprypin
Copy link
Member

oprypin commented Sep 11, 2017

...
Bootstrapping becomes harder for no reason.

If you're gonna be vendoring the dependency, you must do it properly, e.g. by copying the source code or directly downloading it. shards doesn't have to be a part of this.

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.
@akzhan
Copy link
Contributor

akzhan commented Sep 12, 2017

I approve the idea but can't approve embedding of shard that owned not by crystal-lang community.

@icyleaf
Copy link
Contributor

icyleaf commented Sep 28, 2017

Sorry reply late, i'm glad markd may be or will be a part of crystal in anyways.
It is welcome to merged into crystal or move to crystal-lang group.

@asterite
Copy link
Member

@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.

@asterite
Copy link
Member

Actually, it's icyleaf/markd#5 that made everything slower

@akzhan
Copy link
Contributor

akzhan commented Sep 28, 2017

@asterite May you move markd to crystal-lang organization? All things be simpler with it.

@asterite
Copy link
Member

@akzhan No, because to compile crystal we will then needs shards, which was said to be very difficult.

@straight-shoota
Copy link
Member

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 benchmar/source.md) though that comparison is not fair because Markdown simply neglects some markdown features.

@RX14
Copy link
Contributor Author

RX14 commented Dec 13, 2017

This is a stale PR at best

@RX14 RX14 closed this Dec 13, 2017
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.

None yet

7 participants