Code review requested

[Dave Thomas]

Gentle Elixir experts.

I've been working on a pure-Elixir Markdown processor. Today it passed the Gruber tests, so I'm getting close to a first release.

Before I do, I'd love it if anyone with the inclination could pass a critical eye over the code. I'm looking for style issues, idiom issues—anything which could be improved, really.

The code is at https://github.com/pragdave/earmark

Have at it.

Dave

[Wojciech Kaczmarek]

Hi,

I'm not the expert but here's one comment :

For a release, a note in the Readme on how to make CLI executable would be useful (be aware about the change escriptize->escript.build on master)

Best,

Wojtek

--
You received this message because you are subscribed to the Google Groups "elixir-lang-core" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elixir-lang-co...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Josh Adams]

I'm trying to find some substantial bits, but I will say that this not being aligned gives me the heebies for no good reason: https://github.com/pragdave/earmark/blob/master/lib/earmark/block.ex#L52

It's literally all I can see...

--

You received this message because you are subscribed to the Google Groups "elixir-lang-core" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elixir-lang-co...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--
Josh Adams

[Dave Thomas]

On Wed, Jul 9, 2014 at 9:48 AM, Josh Adams <josh.r...@gmail.com> wrote:

I'm trying to find some substantial bits, but I will say that this not being aligned gives me the heebies for no good reason: https://github.com/pragdave/earmark/blob/master/lib/earmark/block.ex#L52

Ugh. I agree. Fixed...  

[Alexei Sholik]

Stylistically it looks ok for the most part.

One thing I would definitely change is extract the mix task from mix.exs. Mix.exs should only ever container your project definition, everything else is noise. The convention is to put mix tasks under lib/mix/tasks/.

If you need a general-purpose padding function to replace this https://github.com/pragdave/earmark/blob/d6b2481d1c573557236e0b3d153729f81fb4ae0e/lib/earmark/helpers.ex#L25-L28, you could use List.to_string(:io_lib.format('~*s', [indent_width, ' '])).

This one makes me cringe a bit: String.slice(str, ignore, 9999999), but it's Elixir's fault. It should have a special value for the argument to mean "until the end".

Good job!

--

You received this message because you are subscribed to the Google Groups "elixir-lang-core" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elixir-lang-co...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--
Best regards
Alexei Sholik

[Dave Thomas]

On Wed, Jul 9, 2014 at 11:14 AM, Alexei Sholik <alcos...@gmail.com> wrote:

Stylistically it looks ok for the most part.

One thing I would definitely change is extract the mix task from mix.exs. Mix.exs should only ever container your project definition, everything else is noise. The convention is to put mix tasks under lib/mix/tasks/.

We had a discussion about this. This task is only for me (it generates README.md from module and function docs) and so it shouldn't be distributed with the main project.

Apparently task aliases will fix this when they arrive. Until then, ejm said to do what I did.

 

If you need a general-purpose padding function to replace this https://github.com/pragdave/earmark/blob/d6b2481d1c573557236e0b3d153729f81fb4ae0e/lib/earmark/helpers.ex#L25-L28, you could use List.to_string(:io_lib.format('~*s', [indent_width, ' '])).

Sure, but this is (presumably) a lot quicker :)

 

This one makes me cringe a bit: String.slice(str, ignore, 9999999), but it's Elixir's fault. It should have a special value for the argument to mean "until the end".

Yeah, I know. I got a funny taste in my mouth when I wrote it :) 

[Alexei Sholik]

We had a discussion about this. This task is only for me (it generates README.md from module and function docs) and so it shouldn't be distributed with the main project.

As it happens, this task is available to anyone who has cloned the project:

$ mix help
...
mix readme            # Build README.md by including module docs
...

I fail to see the validity of "available only for me" argument. if you want to keep it only on your machine, it can be extracted into a separate project and installed as a local archive.

--

You received this message because you are subscribed to the Google Groups "elixir-lang-core" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elixir-lang-co...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Eric Meadows-Jönsson]

A task defined in mix.exs will only be available locally for the project. It will not be available when you use it as a dependency.

Eric Meadows-Jönsson

[José Valim]

We had a discussion about this. This task is only for me (it generates README.md from module and function docs) and so it shouldn't be distributed with the main project.

Exactly.

Another option is to have it in tasks/readme.exs and require it or have a scripts/readme.exs and run it with mix run.

 

If you need a general-purpose padding function to replace this https://github.com/pragdave/earmark/blob/d6b2481d1c573557236e0b3d153729f81fb4ae0e/lib/earmark/helpers.ex#L25-L28, you could use List.to_string(:io_lib.format('~*s', [indent_width, ' '])).

Sure, but this is (presumably) a lot quicker :)

You could also use String.duplicate/2

 

This one makes me cringe a bit: String.slice(str, ignore, 9999999), but it's Elixir's fault. It should have a special value for the argument to mean "until the end".

Yeah, I know. I got a funny taste in my mouth when I wrote it :) 

String.slice(str, ignore, byte_size(str)) or String.slice(str, ignore..-1).

It is better than Listerine ;) 

[Dave Thomas]

On Wed, Jul 9, 2014 at 11:27 AM, Alexei Sholik <alcos...@gmail.com> wrote:

We had a discussion about this. This task is only for me (it generates README.md from module and function docs) and so it shouldn't be distributed with the main project.

As it happens, this task is available to anyone who has cloned the project:

$ mix help
...
mix readme            # Build README.md by including module docs
...

I fail to see the validity of "available only for me" argument. if you want to keep it only on your machine, it can be extracted into a separate project and installed as a local archive.

Only for developers of Earmark itself. It shouldn't be included when used as a dependency 

[Alexei Sholik]

OK. It certainly should be placed under the project definition then if you keep it in mix.exs. I would follow José's guidance and put it in a separate file and require it in mix.exs.

And sorry for hijacking the thread with slightly off-topic matter.

--

You received this message because you are subscribed to the Google Groups "elixir-lang-core" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elixir-lang-co...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Dave Thomas]

String.slice(str, ignore, byte_size(str)) or String.slice(str, ignore..-1).

It is better than Listerine ;) 

As the string might be fairly long, is there an overhead to using these compared to the 999999 approach? I'd love to change it.

Dave 

[José Valim]

Excellent question Dave!

byte_size(str) is fast (O(1)), the other one will traverse the whole string to convert -1 into the proper offset. I have amended the docs of String.slice/2 to make this range property explicit.

José Valim

www.plataformatec.com.br

Skype: jv.ptec

Founder and Lead Developer

--

[Dave Thomas]

byte_size(str) is fast (O(1)), the other one will traverse the whole string to convert -1 into the proper offset. I have amended the docs of String.slice/2 to make this range property explicit.

But doesn't String.slice work on codepoints, and byte_size is bytes? So really it's not too different to using 99999? 

[José Valim]

Yeah, it is the upper limit. String.length(str) <= byte_size(str).

We could also make String.slice(x..-1) or actually String.slice(x..y) for y < 0 and x >= 0 fast too. Someone just need to write those optimizations.

José Valim

www.plataformatec.com.br

Skype: jv.ptec

Founder and Lead Developer

On Wed, Jul 9, 2014 at 7:31 PM, Dave Thomas <da...@pragprog.org> wrote:

byte_size(str) is fast (O(1)), the other one will traverse the whole string to convert -1 into the proper offset. I have amended the docs of String.slice/2 to make this range property explicit.

But doesn't String.slice work on codepoints, and byte_size is bytes? So really it's not too different to using 99999? 

--

[Alexei Sholik]

I'll tackle this.

[Saša Jurić]

I had only time for a cursory glance. Overall it looks very good. Here's a couple of minor nitpicks:

Due to pure chance, I had 0.14.0 in my local shell, and it wasn't compiling. It works on 0.14.2. I think maybe elixir version in mix.exs should point to 0.14.2?

This leads me to the second issue. I opened mix.exs to see Elixir version requirement, and had to search all the way to the end of the file. I agree with Alex that these custom tasks could be moved someplace else. Personally, I've gotten into habit of looking into other projects mix.exs to look for general info (deps, Elixir version, and such). So I prefer simpler mix.exs files that are free of extra code.

Finally, I have mixed feeling about patterns like this. You seem to be generating structs (and in turn modules) only to be able to pattern match on it later. I wonder if this is worth it. Ultimately, the compiled version has over 30 mostly empty modules which exist just for convenience of pattern matching. The number of files probably doesn't play an important role here, but it feels somehow "off".

[José Valim]

Yeah, those would be a good use case for records or just maps, since you probably don't want any polymorphism in it as well.

We would likely model them as discriminated unions, if we supported those, but we don't yet. :)

José Valim

www.plataformatec.com.br

Skype: jv.ptec

Founder and Lead Developer

--

[Dave Thomas]

Well, there were originally maps. But the pattern matching and type checking of structs made the mode far more readable.

In parsers written in Haskell and OCAML, they use types for this. I feel structs are our nearest equivalent.

What's the downside of this approach?

Dave

[Josh Adams]

I love 'overusing' structs.  They feel cozy and my code is very readable.  Downside - might be awkward to interop with from erlang, but hey, not like shared records in an .hrl aren't terrible too :)

Josh Adams