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

WIP: inserting tables with \tableinput command #197

Merged
merged 10 commits into from
Sep 2, 2019

Conversation

cserteGT3
Copy link
Contributor

@cserteGT3 cserteGT3 commented Aug 11, 2019

As discussed in #195 this PR adds the \tableinput{header}{path} command. The current implementation works in the sense that the markdown strings are generated and included in the page. As I see, the followings are need to be done:

  • conversion to html
  • error handling: check if the provided header's length==number of columns
  • tests
  • docs

I need help with the first two tasks (and will do the other two later on):

  1. I just don't know which function to call on the result string to convert it to html.
  2. If the header's and the table's length doesn't match: error should be thrown or an @assert is fine, or should just ignore it and write an error message?

@codecov
Copy link

codecov bot commented Aug 11, 2019

Codecov Report

Merging #197 into master will decrease coverage by 2.55%.
The diff coverage is 2.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #197      +/-   ##
==========================================
- Coverage   87.21%   84.65%   -2.56%     
==========================================
  Files          26       26              
  Lines        1087     1121      +34     
==========================================
+ Hits          948      949       +1     
- Misses        139      172      +33
Impacted Files Coverage Δ
src/JuDoc.jl 100% <ø> (ø) ⬆️
src/converter/lx_simple.jl 45% <0%> (-55%) ⬇️
src/jd_vars.jl 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7572f5...12ab362. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 11, 2019

Codecov Report

Merging #197 into master will decrease coverage by 0.18%.
The diff coverage is 80.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #197      +/-   ##
==========================================
- Coverage   87.21%   87.03%   -0.19%     
==========================================
  Files          26       26              
  Lines        1087     1118      +31     
==========================================
+ Hits          948      973      +25     
- Misses        139      145       +6
Impacted Files Coverage Δ
src/JuDoc.jl 100% <ø> (ø) ⬆️
src/jd_vars.jl 100% <100%> (ø) ⬆️
src/converter/lx_simple.jl 89.47% <80%> (-10.53%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7572f5...cbd9700. Read the comment docs.

@tlienart
Copy link
Owner

tlienart commented Aug 12, 2019

Ok I think your solution is a bit too complicated:

Let's say that the file is test.csv

a, b, c, d
0, 1, 2, 3
4, 5, 6, 7

Then

using DelimitedFiles
content = readdlm("test.csv", ',', String)
open("out.md", "w") do fout
    for row in eachrow(content)
        write(fout, "| " * prod(e * " | " for e in row))
        write(fout, '\n')
    end
end

Does most of the job (header part missing but it's easy to add). Btw, you may have to write rowidx in 1:size(content, 1) because eachrow doesn't work on 1.0 I think)

For your question (1) see review comment, just declare the command in LX_SIMPLE_REPROCESS, nothing else.

For (2), an html_err is probably best so that it doesn't stop the engine but just writes a message on the HTML page.

@cserteGT3
Copy link
Contributor Author

Only reason for this (from the docs):

As illustrated in the above example each column of | characters must be aligned vertically.

If the cells in a column doesn't have the same length (like in the followings), | will not be aligned:

a, b, c, d
string1, 1.123, 2, 3
string126, 0.1, 6, 7

Result markdown:

| a |  b |  c |  d | 
| string1 |  1.123 |  2 |  3 | 
| string126 |  0.1 |  6 |  7 | 

I didn't tested how the parser handles this kind of table, but in the docs it's stated that they must align, so I would rather align them.

@tlienart
Copy link
Owner

tlienart commented Aug 13, 2019

Here's a bit more code which may help with testing etc:

using Markdown

test = """
    This is a table:

    | a |  b |  c |  d |
    | :---: | :---: | :---: | :---: |
    | string1 |  1.123 |  2 |  3 |
    | string126 |  0.1 |  6 |  7 |

    Done.
    """

result = test |> Markdown.parse |> Markdown.html

result = """<style>table, th, td {
  border: 1px solid black;
}</style>\n""" * result

write("/Users/tlienart/Desktop/index.html", result)

if you open that in a browser you'll get:

Screen Shot 2019-08-13 at 09 54 21

which is what we want (separators etc are styled in CSS), so you don't need to have things aligned.

Note: everything is left-aligned even though there's a specification :---: which should produce centered columns; all this can be styled in CSS though afaik so not too much of a problem for now.

By the way, JuDoc also uses this Markdown.parse |> Markdown.html chain though after a bit of processing.

@cserteGT3
Copy link
Contributor Author

I'll try to answer everything in this comment.
My two questions:
As discussed html_err() is used to handle DimensionMismatch of the header and if the file is not present. Therefore I left the command in LX_SIMPLE and used the md2html function to convert the generated markdown to html. Hope that is a good solution.

I experienced too, that alignment doesn't matter for the parser, moreover if | are aligned, the parser can't parse the string into the table, which I don't really understand. I may open an issue, because currently the behaviour is totally different than the documentation 😄

I'll soon figure out the testing, and write the docs.

@tlienart
Copy link
Owner

Hey thanks a lot for this, I'll try to look at this tomorrow and get back to you with, hopefully helpful, comments.

PS: given that you're one of the first to actually have a look at the code, don't hesitate to also let me know the things that don't make sense and I'll try to add comments (or docs) as appropriate to guide future contributions

@cserteGT3
Copy link
Contributor Author

cserteGT3 commented Aug 20, 2019

A quick note:
While I was writing the docs I realized that Documenter needs to have the |-s aligned.
And also the alignment (left-middle-right) works.

@cserteGT3
Copy link
Contributor Author

I've finished the first version of documentation and tests. Let me know if they serve their purpose.

PS: This is my first larger PR, so I'm not sure what else should be done (after approving the changes). Rebasing and/or squashing commits? If so, please do let me know, I'd like to do them to practice open source development 😃 .

@cserteGT3
Copy link
Contributor Author

Answering to my own issue:

I experienced too, that alignment doesn't matter for the parser, moreover if | are aligned, the parser can't parse the string into the table, which I don't really understand. I may open an issue, because currently the behaviour is totally different than the documentation 😄

I started writing an MWE and realized that the parser works totally fine. Also I misunderstood what you've written. Now I understand that CSS is dominant.

docs/src/man/syntax.md Outdated Show resolved Hide resolved
docs/src/man/syntax.md Show resolved Hide resolved
docs/src/man/syntax.md Outdated Show resolved Hide resolved
docs/src/man/syntax.md Outdated Show resolved Hide resolved
src/converter/lx_simple.jl Outdated Show resolved Hide resolved
src/converter/lx_simple.jl Outdated Show resolved Hide resolved
src/converter/lx_simple.jl Outdated Show resolved Hide resolved
@tlienart
Copy link
Owner

Ok, I've added some feedback on "just the code", will now pull your branch, play a bit with it and see if it works as expected, assuming that + fixing of minor comments above, I'll merge. Thanks for contributing!

@tlienart
Copy link
Owner

Ok it mostly looks good, one more thing though: this test:

# header specified
    h = raw"""
        A table:
        \tableinput{A,B,C}{/assets/testcsv.csv}
        Done.
        """ |> seval
    shouldbe = """<p>A table: <table><tr><th>A</th><th>B</th><th>C</th></tr>
            <tr><td>string1</td><td>1.567</td><td>0</td></tr>
            <tr><td></td><td></td><td></td></tr>
            <tr><td>l i n e</td><td>.158</td><td>99999999</td></tr></table>
            Done.</p>"""
    @test isapproxstr(h, shouldbe)

looks incorrect; when the header is specified you should not over-write the first line of the CSV file, so the first row after A, B, C should be h1, h2, h3 which is present in the file

docs/src/man/syntax.md Outdated Show resolved Hide resolved
@cserteGT3
Copy link
Contributor Author

Sorry for the silence, I had a busy week. I've updated the code, tests and documentation (and resolved every review). Please review them particularly the documentation

| 0 | another string | 2.87 |
In this case given no header was specified in the call, a header was generated from the first line in the CSV (here: h1, h2, h3).

If you're file doesn't have a header, you can specify it in the call:
Copy link
Owner

Choose a reason for hiding this comment

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

you're -> your

The look of the table will be defined by your CSS stylesheet.

There's a couple of rules that you have to keep in mind when using the `\tableinput{}{}` command:
* Columns must be separated with comma (`,`).
Copy link
Owner

Choose a reason for hiding this comment

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

with -> by a


There's a couple of rules that you have to keep in mind when using the `\tableinput{}{}` command:
* Columns must be separated with comma (`,`).
* If a header is specified, its length must match the number of columns of the file.
Copy link
Owner

Choose a reason for hiding this comment

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

of the file -> in the file

@tlienart
Copy link
Owner

tlienart commented Sep 2, 2019

looks good to me minus minor nitpicking, thanks a lot for this Tamás! and actually given it's minor, I'll just merge and do it :-)

@tlienart tlienart merged commit afb833e into tlienart:master Sep 2, 2019
@tlienart tlienart mentioned this pull request Sep 2, 2019
27 tasks
@cserteGT3 cserteGT3 deleted the fr195 branch September 2, 2019 15:16
@tlienart tlienart mentioned this pull request Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants