feat: add js, ts, jsx, tsx support - #4
Conversation
jonhoo
left a comment
There was a problem hiding this comment.
Thanks! Let's make a simplifying assumption here ^
jonhoo
left a comment
There was a problem hiding this comment.
Did a more thorough review this time :)
| // and the TypeScript fixture, both of the comment forms their closely | ||
| // -related grammars share are exercised. |
There was a problem hiding this comment.
Why the hard line-wrap here?
There was a problem hiding this comment.
Based on the rest of the code, I thought there was a 100 char line length limit.
There was a problem hiding this comment.
But the line above is not wrapped and is longer?
| // The statement is written as a JSDoc block, which is the shape TypeScript code bases reach for | ||
| // and the one no other fixture in this tree covers. |
There was a problem hiding this comment.
Doesn't panel.tsx exercise this exact same thing?
There was a problem hiding this comment.
👍 Right. I added TS before deciding to add the other languages
There was a problem hiding this comment.
I guess what I mean is that we probably don't need both of those tests then — one would be sufficient.
| return ( | ||
| <aside role="status"> | ||
| {/* | ||
| * YADR: 2024-09-05 Render the banner as a sibling of the page content |
There was a problem hiding this comment.
Instead of inventing new YADRs for each fixture, we should just make all the fixtures (going forward — no need to change the old ones) use placeholders for each parameter position. So, for example, <use case/user story u>, or maybe even just <u>.
| // a block never spans two matches, so `last_line` is only ever compared against comments | ||
| // from the same one. A `(comment)+` pattern groups comments that are *siblings*, which | ||
| // for a run of `//` or `#` lines is the whole run. | ||
| // | ||
| // JSX is the exception, and the reason a Y-Statement written in markup has to sit in a | ||
| // single `{/* ... */}`. Each container wraps its comment in a `jsx_expression` of its own, | ||
| // making a run of them only-children rather than siblings, so tree-sitter reports one | ||
| // match each and they are never grouped. Therefore, spreading a statement over a run of | ||
| // containers is rejected rather than read. |
There was a problem hiding this comment.
This comment isn't particularly useful, and can be cut.
| /// `banner.jsx` writes its statement inside JSX markup, in a single `{/* ... */}` container, which | ||
| /// is the only comment syntax available in a children position and the only shape a statement in | ||
| /// markup may take. Nothing else in the tree covers a statement that is not at the top level of a | ||
| /// file. | ||
| /// | ||
| /// Asserting on the reflowed prose rather than on the title is what makes this worth having: a | ||
| /// title survives almost any mishandling of the container, whereas the paragraphs only come out | ||
| /// whole if the `*` on each line came off and the `{` and `}` around the comment stayed out. |
There was a problem hiding this comment.
The comments on all these test cases (here and on instas.rs) are too much. Most probably don't need a comment at all, or could get away with a single line (or maybe sentence).
| /// `ALL` is what `FromStr` searches and what the unknown-language error lists, so a variant left | ||
| /// out of it is unusable from the command line however well it parses. | ||
| #[test] | ||
| fn all_lists_every_supported_language() { |
There was a problem hiding this comment.
This test isn't useful, since it'd just be yet another place where we'd need to remember to add a language.
| } | ||
|
|
||
| #[test] | ||
| fn typescript_is_selected_for_ts_files() { |
There was a problem hiding this comment.
This feels like a test that doesn't belong in this file (or anywhere).
| Spreading one statement over a run of shorter containers is unsupported: | ||
|
|
||
| ```jsx | ||
| { | ||
| /* YADR: YYYY-MM-DD some title */ | ||
| } | ||
| { | ||
| /* */ | ||
| } | ||
| { | ||
| /* In the context of ... */ | ||
| } | ||
| ``` | ||
|
|
||
| Everywhere else, a run of adjacent comments is read as one block. Each `{/* ... */}` is a JSX expression of its own rather than a plain comment, and how such containers are laid out across lines is decided by whatever formats the file. Grouping them by line would let a reformat quietly change what a statement says, so a statement in markup is kept to one container instead. | ||
|
|
| - Apache License, Version 2.0 | ||
| ([LICENSE-APACHE](LICENSE-APACHE) or <http://www.apache.org/licenses/LICENSE-2.0>) | ||
| - MIT license | ||
| ([LICENSE-MIT](LICENSE-MIT) or <http://opensource.org/licenses/MIT>) |
There was a problem hiding this comment.
This change is unrelated and shouldn't be in this PR.
| content or decision's validity, simply make the necessary corrections without | ||
| updating the date or maintaining a changelog. |
There was a problem hiding this comment.
No need to change these in this PR.
|
Also, this reads a lot like you're using an LLM to assist you in the work. That's totally fine, but I'd then prefer a Co-Authored-By line on the commits (like most agents automatically include) so that we maintain the transparency on what is written by agents :) |
|
I used an LLM for the tests and readme. So, let me know if you would prefer me to rebase with two commits (one with |
No, I think it's fine to just include it with upcoming commits that have that property :) |
410e060 to
9eda65b
Compare
Main Notes
tree_sitter_typescriptdependency addedLANGUAGE_TYPESCRIPTis used to parse JS and TS, because TS is superset of JSLANGUAGE_TSXThe parsing logic made sense until jsx entered the picture withEDIT: Decided to remove support for multi-expression yadr for jsx.{/* Comment */}