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

LuaDoc. Fixed the start position of the comment first symbol in docs #3028

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

TIMONz1535
Copy link
Contributor

The positions were calculated slightly incorrectly there, because it got confused with the lengths.

---@param a string?Comment
local function new(a) end

---@param a string[]Comment
local function new(a) end

now its Comment instead of omment.

изображение

But I came across the fact that the comment texts comment.text do not contain -- at the beginning, because this token is uuuh... being skipped?

That is, we were working with a string of a different length and start pos (-2). I made a variable local textOffset = 2 so that it could be easily fixed in the future.

p.s. Maybe you should figure out why this is happening.

…`---@param a string?Comment` - now its `Comment` instead of `omment`.
script/parser/luadoc.lua Outdated Show resolved Hide resolved
@tomlau10
Copy link
Contributor

tomlau10 commented Jan 5, 2025

But I came across the fact that the comment texts comment.text do not contain -- at the beginning, because this token is uuuh... being skipped?

Regarding this question, I think it makes sense because it stores the body of a comment 🤔

--foo bar
  • the comment body is only foo bar, not --foo bar
  • the prefix -- is not part of the comment body

@TIMONz1535
Copy link
Contributor Author

TIMONz1535 commented Jan 6, 2025

Damn, I found an issues

-- length = comment.finish - comment.start

-- length 26, relative finish 19, actual comment.text `-@param a string?Comment` (24 length, without 2)
-- ok
---@param a string?Comment

-- length 27, relative finish 18, actual comment.text `@param a string?Comment` (23 length, without 4!)
-- also real line length 29! `--` token was skipped before "calculating positions"
-- ok comment but wrong colorizing
--[[@param a string?Comment]]

 -- also wrong colorizing, so no comment issue
--[[@param a string?]]

-- length 33, relative finish 21, actual comment.text `@param a string?Comment` (23 length, without 10!!!)
-- wrong our offset 2, now comment text is `ment` :<
-- also wrong colorizing
--[===[@param a string[]Comment]===]

{20A06A79-94EB-452E-8A70-B9ED68299D84}

I could assume to search for the first character @, but most likely this function is not only for ---@stuff annotations?

Also the expression text:find('%S' seems pointless, because we're already doing util.trim (trimTailComment). Maybe it should have eliminated the empty spaces comments ---@param a string? . but we can never have the end char pos of a string >= comment.finish

@tomlau10
Copy link
Contributor

tomlau10 commented Jan 7, 2025

If you believe this PR is still not complete, I think the first thing is to turn it into draft:
=> this prevents maintainer accidentally merge the in progress PR 🤔 until everything sorts out
https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-stage-of-a-pull-request#converting-a-pull-request-to-a-draft


I will try to respond to the new found issues one by one when I got time 👀

@tomlau10
Copy link
Contributor

tomlau10 commented Jan 7, 2025

the unconsidered comment.long type, worked by chance

-- length 27, relative finish 18, actual comment.text `@param a string?Comment` (23 length, without 4!)
-- also real line length 29! `--` token was skipped before "calculating positions"
-- ok comment but wrong colorizing
--[[@param a string?Comment]]
  • so one of the new issues found is that, for comment.long type, although skipping 2 chars for it works, but the underline principle is different:
    • the actual comment.text is only @param a string?Comment, at first glance we would expect 4 chars are skipped
    • but no, comment.start is 2 in this case, which is what you are saying: "-- token was skipped before "calculating positions"
    • -4 + 2 => it is still -2, so the current convertLinePosToCommentPos() works for this case
  • but it will not work when using nested quote style, in your next example 🐛

not working on nested quote comment

-- length 33, relative finish 21, actual comment.text `@param a string?Comment` (23 length, without 10!!!)
-- wrong our offset 2, now comment text is `ment` :<
-- also wrong colorizing
--[===[@param a string[]Comment]===]
  • 🤔 as for this case, I notice that there is a comment.mark, which is actual extra skipped chars [===[ 💡
  • actually the existing logic already used this value when calculating startOffset 👀
    local startOffset = comment.start
    if comment.type == 'comment.long' then
    startOffset = startOffset + #comment.mark - 2
    end
  • by reviewing the case above --[[@param a string?Comment]], comment.mark = [[
    => this is indeed what we need 😄
    => so we can have a generalized form for the conversion functions pair:
local function convertLinePosToCommentPos(comment, linePos)
    -- some new comment explaining the new formula
    local textOffset = comment.type == 'comment.long' and #comment.mark or 2
    return linePos - comment.start + 1 - textOffset
end

local function convertCommentPosToLinePos(comment, commentPos)
    -- the reverse of convertLinePosToCommentPos()
    local textOffset = comment.type == 'comment.long' and #comment.mark or 2
    return commentPos + textOffset - 1 + comment.start
end

With this fix to both convertLinePosToCommentPos and convertCommentPosToLinePos, now all comments can be recognized correctly now, including your last test case --[===[@param a string[]Comment]===] 👍
But still the coloring for ? after string seems wrong 🤔 and currently I have no idea why.

@TIMONz1535 TIMONz1535 marked this pull request as draft January 7, 2025 13:00
@tomlau10
Copy link
Contributor

tomlau10 commented Jan 7, 2025

Also the expression text:find('%S' seems pointless, because we're already doing util.trim (trimTailComment). Maybe it should have eliminated the empty spaces comments ---@param a string? .

After some thought, this is not pointless 🤔

  • %S means non space characters
  • by using text:find('%S', <comment start offset>), we are filtering out empty comments
    • cstart = nil if no valid comment string is found
    • then the logic will not execute the if case below
    • => therefore we can avoid generating empty result.comment objects
  • otherwise for the case ---@param a string? , even though util.trim can trim all the spaces
    => an useless result.comment will still be generated

but we can never have the end char pos of a string >= comment.finish

If we can be certain that the length of comment.text is always <= comment.finish - comment.start,
then I agree that the convertCommentPosToLinePos(comment, cstart) < comment.finish check is pointless. 👍
As you said, in this case we can never have the converted trailing comment start pos >= comment.finish

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