Skip to content

Conversation

Gazler
Copy link
Contributor

@Gazler Gazler commented Jun 16, 2017

I've done this as two commits as the second commit is technically a breaking change for the way the docs are rendered. You can see the result of of the same table being rendered with:

  1. elixir 1.4.1 (before this feature)
  2. After commit 1
  3. After commit 2

table-alignment

Commit messages below for convenience.


Add table column alignment in IO.ANSI.Docs

This commit adds table alignment in markdown which allows the following
for tables:

right aligned | center aligned | left aligned
-: | :-: | :-
a | b | c

This will output a table in the following format:

right aligned | center aligned | left aligned
            a |       b        | c

Remove duplicate spacings for table alignment

Previously, a table in the format:

right aligned     | center aligned | left aligned
----------------: | :------------: | :-----------
                a |       b        | c

Would be displayed as:

right aligned     | center aligned | left aligned
                a |       b        | c

This commit changes it so it displays as:

right aligned | center aligned  | left aligned
            a |        b        | c

@Gazler Gazler force-pushed the feat/table-alignment branch 2 times, most recently from 1b8af8e to 98dcc44 Compare June 16, 2017 14:23
@@ -291,6 +291,8 @@ defmodule IO.ANSI.Docs do
col =
col
|> String.replace("\\\|", "|")
|> String.replace(~r/-+/, "-")
Copy link
Member

Choose a reason for hiding this comment

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

String.replace(["+", "-"], "-")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@josevalim That's not quite the same thing. My version replaces :------- with :-.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I completely spaced out on the +. We cannot use regexes because it breaks precompiled Elixir on OTP 20. The few cases we do we need to call Regex.recompile. Ideally we want to find a way to write this that does not rely on regexes.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a chance this code will replace foo -- bar in a column to foo - bar?

Copy link
Member

Choose a reason for hiding this comment

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

Hrm, it will. I think we need to do this squashing only after we detect the first line is a table_header?

Copy link
Contributor Author

@Gazler Gazler Jun 16, 2017

Choose a reason for hiding this comment

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

@josevalim We can probably do it by excluding the header lines from the length count. That gets rid of the regex and the need to trim down the "-".

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me.

Enum.map_join(cols_and_widths, " | ", fn {{col, length}, width} ->
col <> String.duplicate(" ", width - length)
Enum.map_join(cols_and_widths_and_aligns, " | ", fn {{{col, length}, width}, align} ->
case align do
Copy link
Member

Choose a reason for hiding this comment

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

Let's move this case to a function as things are getting wild here!

@@ -303,6 +303,12 @@ defmodule IO.ANSI.DocsTest do
"\e[7mcolumn 1 | and 2\e[0m\na | b \none | two \n\e[0m"
end

test "table with heading alignment" do
table = "column 1 | 2 | and three\n-----: | :------: | :-----\n a | even | c\none | odd | three"
expected = "\e[7mcolumn 1 | 2 | and three\e[0m\n a | even | c \n one | odd | three \n\e[0m"
Copy link
Member

Choose a reason for hiding this comment

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

Can we use heredocs for those strings? You just need to append <> "\e[0m" at the end I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can, I was just following the style of the other tests in the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  test "table with heading alignment" do
    table = """
    column 1 | 2        | and three
    -------: | :------: | :-----
        a    |  even    | c\none | odd | three
    """

    expected = """
    \e[7mcolumn 1 |   2   | and three\e[0m
           a | even  | c        
         one |  odd  | three    
    \e[0m
    """ |> String.trim_trailing

The expected doesn't look greate with heredoc because of the \e[7m and the required trailing whitespace at the end. I can keep it if you want though.

Copy link
Member

Choose a reason for hiding this comment

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

Good point. It is ok to not do it then, your call.

@Gazler
Copy link
Contributor Author

Gazler commented Jun 16, 2017

@josevalim Please approve if it's ok and I'll squash the commits and rewrite the commit message to be more appropriate.

This commit adds table alignment in markdown which allows the following
for tables:

    right aligned | center aligned | left aligned
    -: | :-: | :-
    a | b | c

This will output a table in the following format:

    right aligned | center aligned  | left aligned
                a |        b        | c

If headings are set, the header line (the line immediately below the
headings is ignored when calculating the max width for the columns.
All lines are trimmed of additional whitespace leading and trailing
whitespace to prevent tables that look like:

    right aligned | center aligned | left aligned
    ------------: | :------------: | :-
         a        |        b       | c

    right aligned | center aligned  | left aligned
         a        |      b          | c
@Gazler
Copy link
Contributor Author

Gazler commented Jun 16, 2017

@josevalim All ready to merge now.

@Gazler Gazler force-pushed the feat/table-alignment branch from d4bdda6 to f29796f Compare June 16, 2017 20:47
@fishcakez fishcakez merged commit a6ed2bf into elixir-lang:master Jun 18, 2017
@fishcakez
Copy link
Member

Thank you ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants