feat: add support for HTML, Markdown and Typst files #127
Conversation
CodSpeed Performance ReportMerging #127 will not alter performanceComparing Summary
|
|
Any idea what's going on with the tests? |
There was a problem hiding this comment.
Hi @Rolv-Apneseth, thanks for your PR!
See my comments and let me know what you think :-)
| "md" | "mkd" | "mdwn" | "mdown" | "mdtxt" | "mdtext" | "markdown" => { | ||
| FileType::Markdown | ||
| }, | ||
|
|
||
| "html" | "htm" => FileType::Html, |
There was a problem hiding this comment.
Any documentation or link that documents those many different extensions for the same filetype?
There was a problem hiding this comment.
I believe I just copied this list from https://superuser.com/a/285878. Maybe I should also add mdx as mentioned in a comment, or do you want to cut it down to just md and markdown?
There was a problem hiding this comment.
I think it's better to either stick to simplicity, or to match the default behavior of some popular tool, like ripgrep's default list.
There was a problem hiding this comment.
Alright, well I'll copy that list then
There was a problem hiding this comment.
Wouldn't be better to return a vector of data annotations here? So (1) we avoid memory allocation and (2) we can benefit from LT's support for annotated data. The latter also has the advantage that LT is going to compute the correct location of an error in the text, and we can then print annotated errors with respect to the raw content of the file.
There was a problem hiding this comment.
I'm not too familiar with how the data annotation side of things work, and I had issues trying to use it with the current setup that splits the requests, but I can explore it a bit more if you want.
Some notes / questions though:
- Should all requests sent by the CLI for files be data annotations, including text, or does that need to be handled separately to the other file types
- The requests will be a lot larger than the current plain text ones right? So only much smaller files would be supported
- I will also need to change the current approach for markdown files as that wouldn't make sense any more
There was a problem hiding this comment.
- I think all special types should be checked using data annotation. That excludes raw text files.
- That can be an issue with very large files, but let's not bother too much on that. That can be solved by automatically splitting the request into many (which can itself be a problem if we send too many requests to the public API, but people should host then own server).
- Probably, yes. Let me know if I can help!
| #[default] | ||
| Auto, | ||
| /// Raw text. | ||
| Raw, | ||
| /// Markdown. | ||
| Markdown, | ||
| /// HTML. | ||
| Html, | ||
| /// Typst. | ||
| Typst, | ||
| } |
There was a problem hiding this comment.
This doesn't have to be fixed / changed in this feature, but what do you think of the following:
- add a
Textvariant that matches.txtfiles; - ignore
Rawfile types by defaults (because some tools would probably callltrswill many files as input, likepre-commit, and it doesn't make sense to check for errors in files like.zip, code files, or else); - add CLI options
--include-rawand--include=extwhere the latter includes files withextextension, usingRawlogic.
?
There was a problem hiding this comment.
Sounds alright yeah. Maybe it shouldn't be "raw" then, more just "unknown" or "unsupported", what do you think? I was thinking "raw" just because it would be sent as-is, but if there's a separate "text" option I think that would be more clear
There was a problem hiding this comment.
Yes, I think Unknown sounds perfect! I agree that Raw conveys the message greatly, my main point was about not checking arbitrary file types, at least by default.
There was a problem hiding this comment.
All good. I can do this in a separate PR after this one then.
| "code" => { | ||
| txt.push_str("_code_"); | ||
| return; | ||
| }, | ||
| "a" => { | ||
| txt.push_str("_link_"); | ||
| return; | ||
| }, | ||
| "pre" => { | ||
| txt.push_str("_pre_"); | ||
| txt.push_str("\n\n"); |
There was a problem hiding this comment.
It is a bit linked to my general file-comment, but there you could simply return a Markup note, with no interpret_as, and LT should ignore it, I think.
Hey @jeertmans, I'll get back to this in a couple days, thanks for the review though |
No issue, Merry Christmas and happy end of year! |
Thanks, you too :) |
Looks strange! Sometimes, the online version of the LT API returns different results than the one you can host locally. Did you run tests on a self-hosted API? |
Yes, it was with a self-hosted API. Looks like these errors are on the current v3 branch too though (#117), so I don't think these changes are related. |
Continuing #117, as per our discussion.
This PR adds parsers for HTML, Markdown and Typst files, which are used in the CLI to support different file types. Auto detection per file is used by default, which will just choose a parser based on the file's extension.
I think the parsers are working OK, but wanted to open this PR as soon as possible so you can let me know what you think, and to see if you have some files that maybe this doesn't work great for.
I decided to take the same path that pyLanguagetool took for Markdown files, by first converting them to HTML, and then using a separate library for parsing that (html_parser in our case). I figured we'd probably want support for HTML eventually anyway, but you can let me know if you think I should take a similar approach to the HTML and Typst parsers.
As for the HTML and Typst parsers, I've inserted some "placeholders" in place of e.g. code blocks, links etc., which
LanguageToolignores, e.g._code_,_math_,_link_. This seems to work well enough but if you have any other ideas on how to tackle these let me know and I can have a look.