Repository navigation
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds tracking for the offset and length of inline code spans parsed from top-level paragraphs or ATX headers, allowing their original source locations to be recovered. This is achieved by introducing a ContentOffsetMapper class and updating several syntax parsers, AST nodes, and utility functions to propagate offset information. Comprehensive tests have been added to verify the tracking behavior under various scenarios. There are no review comments, so I have no feedback to provide.
|
@srawlins, do I need two reviews to land this here too? |
|
So sorry, I gave this a cursory review and didn't send. |
…dexing Makes UnparsedContent's offsetMapper a named parameter for easier incremental changes, and clarifies that Line.offset is 0-indexed. Co-Authored-By: Claude Sonnet 5 <[email protected]>
github-actions
Bot
added
type-infra
cfda4cd to
45846ac
Compare
github-actions
Bot
removed
type-infra
|
Sorry everyone. I rebased and my fork was at the incorrect commit so all my rebase changes were considered changes in this PR. I've reverted it back and am now back to 11 files but I can't remove any reviewers. Ready for your new review, @srawlins. |
|
/gemini review @FMorschel - FYI: do NOT take gemini's comments as gospel. But please carefully consider them. |
There was a problem hiding this comment.
Code Review
This pull request introduces tracking of the original source document offset and length for inline code spans parsed from top-level paragraphs or ATX headers. This is achieved by introducing a ContentOffsetMapper and updating the parsing logic across blocks, lines, and inline syntaxes. Feedback is provided to add a documentation comment to the public constructor of ContentOffsetMapper to comply with the repository style guide.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
|
||
| /// The length, in the original source document, of the text that produced | ||
| /// this element, or `null` if it is not tracked for this kind of element. | ||
| int? length; |
There was a problem hiding this comment.
Sadly, neither Element or Node are final classes, and they are in public API right?
So this is a breaking change for anyone implementing these classes.
In an ideal world I imagine we would use @deprecated.subclass/implement, and then we make them final in a new major version of package:markdown.
I think I'm open to landing a PR that tries to mark pretty much all classes as deprecated for subclassing, etc.
But it's also possible we should rip the band-aid off and mark things sealed and final.
This is kind of a non-trivial migration, and orthogonal. But something we should consider before adding new properties.
Side note: given how much of package markdown is public and completely broken if you where to combine parsers in arbitrary ways I could be convinced we should maybe just accept the breakage.
There was a problem hiding this comment.
For the record:
I've often found myself wanting this feature, because I want to programmatically edit markdown :)
Or hang analyzer lints on markdown entities that do something I don't like (one can dream right, hehe).
There was a problem hiding this comment.
I'm fine with whatever you decide, I didn't think about this when implementing, but yes, you're right. Although even though it is public, I don't suspect most people would have many reasons to implement any of these classes? Maybe testing but I think that is probably it? I don't suppose this is a major problem in any case, but I'm fine with landing a PR for deprecating them in that way or to make them final.
There was a problem hiding this comment.
Given that this dart team package it's subject to:
So probably we have to roll into dart SDK deps which then rolls into google3... I think package markdown is used in the dart SDK repo, so it probably rolls in with it.
If we want to grash how hard deprecating the ability to subclass all these things we could just try to land in a PR, roll it into the Dart sdk DEPS file and see if we can land that, or if it gets push back. If it sticks we can try to do a change that marks all the classes final.
Making the Node class sealed so it easy to switch on subclasses would be pretty cool.
This is all just shoring up, so that people don't subclass things in package:markdown that we don't them to.
Probably two PRs and two CLs and some time as try to roll it through DEPS and have it stick there. Maybe an issue outlining the plan. I could see us focusing on just Node and it's subclasses.
I have a more love/hate relationship with how everything in package markdown can be subclassed.. its powerful that you can subclass a block parser for a code block, but from a compatibility perspective it's completely impossible to make any changes here without breaking someone.
There was a problem hiding this comment.
Oh, and about lints on markdown, @jonasfj, I'd suspect that is very much doable today. We currently don't handle inline things like this (code) or bold/italics, that I'm aware of, but whatever this package can parse, we should be able to handle.
This PR came after:
So we currently use the parsed information to supply semantic tokens on DAS. Probably some of these things are fine to be used in an analysis_server_plugin.
Fixes part of #1287.
This is the initial work that covers tracking the offset/length of inline code spans. See what is missing at #1287 (comment).
Replaces the incorrectly based PR: #2529
Contribution guidelines:
dart format.Many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.
Note: The Dart team is trialing Gemini Code Assist. Don't take its comments as final Dart team feedback. Use the suggestions if they're helpful; otherwise, wait for a human reviewer.