straight-shoota commented 7 years ago

The Markdown parser and renderer is still very basic and misses important features like for example raw html.

I suggest to implement the CommonMark specification which is very reasonable and the de-facto standard reference for Markdown implementations. There is an entire test suite jgm/CommonMark to validate implementations. Currently Markdown.to_html passes only 137 of 621 (22 %).

It would also be nice if the Markdown parser/renderer can be easily extended to add support for more specialized features like Github Flavoured Markdown (e.g #2217)

straight-shoota commented 7 years ago

Yes, crystal deps should be removed because it's a different tool called shards. But I'd still think it would be great to have compiler tools - even from external binaries - transparently integrated into a single CLI. The user doesn't even have to know he is actually running crystal-doc.

oprypin commented 7 years ago

I don't like moving crystal doc to a separate repository because that can limit development of advanced features in it, and/or require unnecessary work on making the compiler tailored for it. It's just not time for that yet. Other than that, sounds great. Maybe worth doing closer to 1.0?

Having the compiler depend on a separate repository (markd) is, I think, a dismissed option at this point. In addition to what was said before, it has a blocking technical reason behind it -- it makes it impossible to introduce breaking changes in the compiler.

Vendoring the code of that repository (copying and periodically syncing it without allowing direct changes to it) would also fail due to the same problem.

I'd like to see Markd integrated with the standard library, but maybe it's not ready for that, and maybe we don't want Markdown in standard library anyway.

In IRC @RX14 suggested another option -- copying and periodically syncing the source code of Markd to this repository, and directly editing (a.k.a. forking) it only if absolutely necessary. I argued against it (what about maintenance and what about distro packaging), but the concerns turned out to be minor. It is a simple enough option that would work.

RX14 commented 7 years ago

I still don't mind the splitting option, for the record, I just think it might be a bit too much work.

faustinoaq commented 7 years ago

Maybe we can create a shard crystal-lang/crystal-utils to make compiler std more lightweight.

RX14 commented 7 years ago

Just as another point of data, rust uses cargo to build it's compiler out of modules. Those modules frequently depend on crates outside of the tree, and as far as I can see there's no vendoring. I think using shards will be fine after 1.0, when we can guarantee that we don't have to fix the shard code halfway though. Before that though, why not simply copy the release of markd we want into the compiler source and then edit it if we have breaking changes to the compiler?

j8r commented 6 years ago

I think it's wiser to use a markd shard forked in crystal-lang/markd directly.

This way we can be confident about breaking changes and be able to send/pull commits from the original project.

straight-shoota commented 6 years ago

It would really be nice to get this going forward. The limitations of the current markdown parser are holding back improvements of the API docs. It would be helpful in many places to use tables and sometimes direct HTML markup.

j8r commented 6 years ago

The main problems are:

the parser is rather simplistic and doesn't support features like numbered lists
the parser isn't likely used much outside the compiler, those wanted a full markdown support use external shards like markd
the presence of markdown in the stdlib is questionable. I don't know language doing this, and why choosing this markdown vs another flavored markdown vs restructured text or any other markup language?

Solutions

Parsing markdown is only used in the doc generation, which is rendered as HTML. Using a JS library like marked which parses markdown to render HTML will do the job.
using an external shard can also be considered, but the opinions on how to do this are mixed. Furthermore, as said above, I dont't think a markdown parser is in its right place in the stdlib.

straight-shoota commented 5 years ago

Maybe one step we could agree on for now is to remove Markdown from the stdlib and change it to a private internal implementation for the compiler (as proposed in https://github.com/crystal-lang/crystal/issues/4613#issuecomment-328351783).

We all agree that the current Markdown implementation is far from complete and lacking in many ways. It does not accord to the quality usually expected of stdlib libraries. It's better to provide no Markdown library than an ineffectual one.

We could simply advise to use markd instead. While not essential, it would probably be a neat corporate action to transfer markd to crystal-lang org. This would express official support for the replacement of the former stdlib library. I think this would be a win for everyone. What do you think @icyleaf ?

icyleaf commented 5 years ago

Sure, I will to contribute mard to Crystall community. 🖖

-- /**

About me
@author icyleaf
@url http://icyleaf.com
@version always beta */ 2018年12月3日 +0800 06:33 Johannes Müller ，写道：

Maybe one step we could agree on for now is to remove Markdown from the stdlib and change it to a private internal implementation for the compiler (as proposed in #4613 (comment)). We all agree that the current Markdown implementation is far from complete and lacking in many ways. It does not accord to the quality usually expected of stdlib libraries. It's better to provide no Markdown library than an ineffectual one. We could simply advise to use markd instead. While not essential, it would probably be a neat corporate action to transfer markd to crystal-lang org. This would express official support for the replacement of the former stdlib library. I think this would be a win for everyone. What do you think @icyleaf ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

asterite commented 5 years ago

So we recently moved Markdown from a public place to an internal compiler source.

We are now in this state (~ is "meh", - is negative, + is positive):

(~) there's no Markdown in the standard library
(-) the docs command uses a buggy, incomplete Markdown implementation

We can choose one of the following:

1. Do nothing

(~) no Markdown in std but people can use markd or common-cmark for Markdown in their code
(-) docs doesn't support the full markdown spec and it's buggy

2. Copy markd to an internal compiler location

I recently send a PR to markd with some optimizations. It's now about 2.5x times slower than crystal-cmark, which I think is acceptable.

markd passes the official Markdown spec suit.

(~) there's still no Markdown in the standard library but people can use markd or common-cmark for that in their projects
(+) docs supports the entire Markdown spec

3. Copy markd to the standard library

(+) there's Markdown in the standard library. I believe this is good because having it here means everyone will probably use it. You won't end up with a project that, through transitive dependencies, depend on multiple markdown implementation. Plus the entire spec already passes and its performance is pretty good. It has options for GitHub flavored markdown and many others. And through usage we can continue improving it.
(+) docs supports the entire Markdown spec

What do we do?

I prefer the last option.

In options 2 and 3 we'd move markd to the compiler's source and it means running more specs. I think what we can do is place these libraries inside a spec/lib/... directory and run those specs separately from the std. In fact we could do that for JSON, YAML, CSV, XML and many others: treat them as shards/libraries, even though they are inside the standard library. Then we'd split the spec runs in CI for each of those libraries, keeping compilation times and memory consumption low.

Otherwise, we could start with option 2 and if it turns out to work really well we could promote it to option 3.

I understand that we could continue discussing whether we want the compiler to depend on shards and how to do that. But right now the state is not good (bad docs and no Markdown) so we can improve that situation first and then take care of how we organize the compiler.

bcardiff commented 5 years ago

👍 for option 3 🥇if the main api with respect to what we used to have in the std is not changed

straight-shoota commented 5 years ago

Just to remove some bias from the ++ third option, I'd like to mention some other metrics 😄

1.

(+) No added complexity. 🤷‍♂
(+) Markd can iterate independently of Crystal releases.

2.

(~) A little bit more complexity for the compiler, but don't need to maintain and spec it separately.
(+) Markd can iterate independently of Crystal releases and only sync the code from the shard occasionally.

3.

(-) More work for core team to maintain Markdown in the stdlib when it could just be developed independently.
(~) Markd releases are tied to stdlib releases, so new features might not propagate as quickly as they could.

IMO I don't think 1 is a good option. 2 would be acceptable. It would essentially add a dependency on a shard, but with the source code vendored into the Crystal repo.

I do prefer 3, though. Even if it's not strictly required to have Markdown available as a tool in stdlib, I think it's a great feature. I remember when I started working with Crystal, I was pleasently surprised to see a Markdown implementation in the stdlib - apart from other useful tools (it was only later that I learned how buggy it was. But the intention counts 😆 ). The API for Markdown is pretty slim and there are no huge demands for customizability, so it's doesn't add a lot of edges.

I'd definitely support compartmentalizing individual libraries inside the standard library.

asterite commented 5 years ago

🥇if the main api with respect to what we used to have in the std is not changed

Definitely! That's something I thought too. Or at least the main API which is just to turn some text into HTML (I think custom renderers will have to change, but mainly because the old std code didn't take some things into account).

I do prefer 3, though. Even if it's not strictly required to have Markdown available as a tool in stdlib, I think it's a great feature.

Cool! I'll try to work on this then when I have time. It should be relatively simple: just copy the code and rename some things. The hardest part would be to fit the current docs renderer, which has some custom logic to highlight code and link types and methods, into markd's renderer. But it shouldn't be that hard.

oprypin commented 5 years ago

Regarding "Copy markd to the standard library": Might be nice to preserve all the Git commits. Or it might not be, maybe there are some caveats.

The result: https://github.com/crystal-lang/crystal/compare/master...oprypin:markd (see at the bottom)

Here's how to do it:

git remote add markd https://github.com/icyleaf/markd
git fetch markd master
git checkout -b markd-move FETCH_HEAD
git rm -r .circleci .vscode .gitignore .travis.yml shard.yml 
mkdir -p spec/lib/markd
git mv spec/fixtures spec/*.cr spec/lib/markd/
git mv *.md LICENSE src/markd/
git commit -m "Move files to prepare merging into Crystal stdlib"

git remote add origin https://github.com/crystal-lang/crystal
git fetch origin master
git checkout -b markd FETCH_HEAD
git merge --allow-unrelated-histories markd-move -m "Merge https://github.com/icyleaf/markd"

j8r commented 5 years ago

What's wrong with git subtree? It would be good to not link markd versions with Crystal releases. This isn't a good reason enough to expose a third party library just because it's used by a compiler tool. One can choose a library version, update it (and gain features/security patches) independently of the Crystal language.

straight-shoota commented 5 years ago

Git subtree would be option 2 (technically, it could also be used with 3, but I don't think that makes much sense). Moving it entirely into stdlib has the benefit of having it available to every Crystal program.

j8r commented 5 years ago

What would be the API? If it's the same as markd (Markd::), it will conflict with the one installed with shard.yml. If different, a fork will likely be needed. It is not an option to force every users bumping the library version when bumping the Crystal compiler version too.

asterite commented 5 years ago

@j8r markd is already feature complete and passes all specs. There haven't been recent commits that fix stuff in the library, just optimizations (it seems) or new features. So I think moving this to the standard library is good:

it's unlikely to change that often
it's available to all users without using an extra shard
having it in std means it's more likely to be used and noticed and so more bugs and performance optimizations could be solved/added

straight-shoota commented 5 years ago

@j8r I agree that having libraries tied to Crystal's stdlib release schedule can be problematic. But as @asterite said, it's a huge issue regarding Markdown. There are already other libraries in the stdlib (HTTP, SSL for example) that would benefit much more from having individual releases.

In the future we might eventually ending up with the stdlib consisting of essentially a set of standard shards that come installed with crystal by default, but individual projects can use different versions of them (installed in local lib). We don't have that setup now, and I don't think it's urgent before 1.0. But we'd like to have markdown support in stdlib, so the best solution currently is to merge markd.

j8r commented 5 years ago

Ok that may be fine if you consider the markdown implementation very stable. In fact, the main issue I find is to put everything possible (I exaggerate) in the crystal/crystal monorepo.

Can we still keep in its own repo, and include it with subtree (or whatever) in the stdlib (option 3)? This would be a shame that @icyleaf, the markd creator, won't be able to review PRs and contribute to the stdlib's markdown. Furthermore, the owners will have to backport changes upstream and downstream. For the record, Golang and Rust do this division in some extent (not with subtree, though).

asterite commented 5 years ago

This would be a shame that @icyleaf, the markd creator, won't be able to review PRs and contribute to the stdlib's markdown

Why not?

Furthermore, the owners will have to backport changes upstream and downstream

I imagine markd will be moved to the std and then it wouldn't make sense for markd to continue existing beside the std.

Can we still keep in its own repo, and include it with subtree (or whatever) in the stdlib (option 3)?

This only affects Crystal std/compiler developers, so just a bunch of people compared to the entire Crystal userbase. So I personally don't see this decision as having a lot of importance and it could always be tackled later.

j8r commented 5 years ago

Why not?

Because he won't be the owner of the repo anymore, just another Crystal contributor.

I imagine markd will be moved to the std and then it wouldn't make sense for markd to continue existing beside the std.

Not really. It would also be harder to contribute than if markd lives in its own repo. Review would be longer, CI too, the crystal-lang/crystal have more setup requirements. Wanting a custom markdown parser to build our own flavor, or implement an existing one, will require to fork the whole Crystal repo (EDIT: or advanced git-fu).

bcardiff commented 5 years ago

If there is a chance for the markdown module to be available directly in the std-lib I would like to avoid having a release without it. Right now 0.31.0 might come out without a markdown module. That will cause dependencies to move out of the std lib since there is no deprecation in place.

While we settle how/if do this, should rollback #8115 to avoid reverting a breaking change in a release?

straight-shoota commented 5 years ago

@j8r You're making generally valid points and more separation might be a good path for the future. But currently, we have a monolith repository. And I think this is fine for now and we don't need to change this setup. That would require a lot of effort and simply isn't necessary at the moment.

Besides, your argument only targets markdown, because the markd implementation is currently a separate shard, suggested to be moved into stdlib. It has not been a part of the stdlib before now, but the same is true for to many other standard libraries. The only difference is, they're currently already in stdlib. I don't think it makes sense to go a separate way right now just for markdown.

@bcardiff Yes, we should avoid breaking markdown in 0.31.0. But I suppose, if we settle on @asterite's proposal, Markd could be imported rather quickly. If we target for a release at the beginning of October, this should work out.

asterite commented 5 years ago

Another way to see it: we can import markd into the standard library and rename it to Markdown. markd can still exist and you can use it instead of that if you really want to. Markdown can be seen as the markdown implementation provided with the std, whose updates follow the Crystal release cycle and contributing to it is "slower" than contributing to markd (which might not be true because here we are many while in markd it's just one). It just happens that right now Markdown has the same implementation as markd, or uses that under the hood.

asterite commented 5 years ago

I think I won't have time to do this, so if someone can take over I'll be very grateful 🙏

asterite commented 5 years ago

If someone wants to tackle this, they will need to:

copy markd by moving it to the standard library under src, but specifically the code in this PR. Include the specs and put them in spec/src/markdown (I think the specs are very simple and short so we can include them there). Make sure to put the helpers in that same file as private methods.
- rename Markd to Markdown
- avoid reopening HTML and overriding decode_entities and others
- implement the custom doc renderer by somehow subclassing Markd::HTMLRenderer: the crystal docs renderer will highlight code and do some links in code sections
- remove the old internal Markdown module

icyleaf commented 5 years ago

@j8r It's alright to move markd to crystal-lang organization, I believe this move will make markd and crystal both better :)

asterite commented 5 years ago

Actually, let me try once. more today. Yesterday I bumped into a compiler bug that was recently reported and I desisted, but maybe I can find a workaround.

bew commented 5 years ago

Which bug was it? (maybe someone can have a try at it while you have fun with markd?)

asterite commented 5 years ago

Ah, nevermind the bug. I somehow copied the text from https://github.com/crystal-lang/crystal/issues/8163#issuecomment-529963516 into the markdown code and it was exploding because of that. No idea how that happened.

Later today I'll submit the markd inside std PR. It won't have the git history, sorry. But the history can be kept in markd's repo.

asterite commented 5 years ago

I have the code but I won't send the PR. It seems Brian and Juan agreed that it might not be good to have Markdown in the standard library and that we should move crystal docs to an external tool where we could use external shards there.

So for now it might be better to just do nothing here.

bcardiff commented 5 years ago

We were iterating with @waj on what to do here, our proposal would be:

fork icyleaf/markd to crystal-lang/crystal-markdown
- keep @icyleaf as maintainer of crystal-lang/crystal-markdown
- rename markd to use the top level Markdown namespace that was available before
- check that the main api from Crytstal 0.30.0 is honored by crystal-lang/crystal-markdown
encourage users to include crystal-lang/crystal-markdown as a dependency if needed in the Crystal 0.31.0 changelog
Delay for 0.32.0 the discussion on how to use crystal-lang/crystal-markdown in the compiler
Leave the docs tool as is for 0.31.0, with it existing bugs

This way we avoid copying the whole shard, allow @icyleaf to keep iterating, offer migration from Crystal 0.30 to 0.31, aim for reducing the std-lib size and maybe split the compiler.

asterite commented 5 years ago

fork icyleaf/markd to crystal-lang/crystal-markdown

What's the purpose of that? Why can't markd continue existing as a shard on its own? Why do we need to become its owners?

bcardiff commented 5 years ago

Avoiding dependencies outside the organization is a safe policy for avoiding them to disappear. Is easier to keep permissions for the core-team.

j8r commented 5 years ago

An option for the future would be to have the markdown library as a dev dependency (that could be bundled, or not, in the release). In fact, generating the API docs is optional, and it could even be possible to generate docs with no markdown. This would prevent the chicken and egg issue @ysbaddaden was pointing out if the library becomes a hard requirement.

bcardiff commented 5 years ago

In the future the docs tool might even get out of the compiler directly. Yet bundled in the distribution packages. Doing that could also remove the chicken and egg situation of shards used in the compiler if needed.

Again, I want to focus on what the users should use as a replacement of the moved Markdown in order include that in the release 0.31.

We can focus on how to improve the docs with better markdown support later.

straight-shoota commented 5 years ago

@bcardiff If we want to postpone the decision to after 0.31.0, I suggest to hold off on moving markd to crystal-lang/crystal-markdown as well. That's a change that looks good and might be done anyway, but maybe we come to a different solution. I'd rather avoid it until the immediate goal is settled. Postponing would also mean to revert the removal of stdlib's `Markdown' for 0.31.0.

bcardiff commented 5 years ago

That would be a full postpone. :-) But I'm fine with it.

While we settle how/if do this, should rollback #8115 to avoid reverting a breaking change in a release? https://github.com/crystal-lang/crystal/issues/4613#issuecomment-529927441

asterite commented 5 years ago

I think this is all just a big mistake. Markdown in docs has a very tiny priority. The next big steps are getting parallelism tried out and done well and Windows, not focusing on extracting tools outside of the compiler, which is transparent for users and probably a maintenance burden for us and Crystal distributors.

But improving Markdown is already here without a cost at all (I already sent a PR) and brings an immediate improvement without breaking any API (the main Markdown.to_html method is still there).

If I never continued commenting on this issue this entire "let's wait and extract compiler tools" discussion wouldn't even exist.

But do as you wish.

crystal-lang / crystal

Improve Markdown implementation - Or: Should we separate compiler tools? #4613

Solutions

1. Do nothing

2. Copy markd to an internal compiler location

3. Copy markd to the standard library

What do we do?