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

LSP Folding Ranges #79

Open
wants to merge 9 commits into
base: anytext
Choose a base branch
from
Open

LSP Folding Ranges #79

wants to merge 9 commits into from

Conversation

nhett
Copy link
Collaborator

@nhett nhett commented Jan 7, 2025

No description provided.

@nhett nhett requested a review from georghinkel January 7, 2025 11:41
@nhett nhett self-assigned this Jan 7, 2025
AnyText/AnyText.Core/Parser.FoldingRanges.cs Outdated Show resolved Hide resolved
AnyText/AnyText.Core/Parser.FoldingRanges.cs Outdated Show resolved Hide resolved
AnyText/AnyText.Core/Parser.FoldingRanges.cs Outdated Show resolved Hide resolved
AnyText/AnyText.Core/Parser.FoldingRanges.cs Outdated Show resolved Hide resolved
AnyText/AnyText.Lsp/LspServer.FoldingRanges.cs Outdated Show resolved Hide resolved
AnyText/AnyText.Lsp/LspServer.FoldingRanges.cs Outdated Show resolved Hide resolved
Assert.IsNotNull(parsed);
Assert.That(parser.Context.Errors, Is.Empty);

parser.Update([new TextEdit(new ParsePosition(10, 0), new ParsePosition(10, 0), ["\r\n"])]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Der Inhalt der Zeile sollte keinen Zeilenumbruch enthalten, sonst sieht AnyText den nicht. Wenn dann lieber zwei leere Strings im Array

AnyText/Tests/AnyText.Tests/CommentChangeTests.cs Outdated Show resolved Hide resolved
{
var commentRuleApplication = ruleApplication.Comments[i];

if (commentRuleApplication.Rule is MultilineCommentRule)
Copy link
Contributor

Choose a reason for hiding this comment

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

ich würde hier auch gern lieber eine Methode in der CommentRuleApplication vorsehen wollen

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Eine separate RuleApplication Klasse für Comments existiert nicht, ich kann aber schauen eine solche für diesen Zweck umzusetzen.

@nhett nhett requested a review from georghinkel January 9, 2025 13:11
Copy link
Contributor

@georghinkel georghinkel left a comment

Choose a reason for hiding this comment

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

Sieht gut aus

@@ -161,6 +163,48 @@ public override object GetValue(ParseContext context)
}
}

public override void GetFoldingRanges(ICollection<FoldingRange> result)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ich glaube jetzt sollten wir die Methode auch umbenennen. GetIrgendwas klingt nach Query, es ist jetzt aber ein Command

{
base.GetFoldingRanges(result);

if (Rule is ZeroOrMoreRule zeroOrMoreRule && zeroOrMoreRule.InnerRule.IsImports())
Copy link
Contributor

Choose a reason for hiding this comment

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

Ich hatte gemeint, dass wir für diese Sequenz eine virtuelle Methode in Rule vorsehen, die dann für ZeroOrMoreRule, SequenceRule und ParanthesesRule passend überschrieben wird. Aber dann wird es schwieriger, die Methode GetFoldingRange hier wieder zu verwenden.

StartLine = (uint)Inner.First().CurrentPosition.Line,
StartCharacter = (uint)Inner.First().CurrentPosition.Col,
EndLine = (uint)Inner.Last().CurrentPosition.Line,
EndCharacter = 0, // determining the end character using length or examinedTo is inconsistent and can lead to wrap around when casting to uint
Copy link
Contributor

Choose a reason for hiding this comment

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

examinedTo ist eine ganz schlechte Idee, aber Length sollte eigentlich funktionieren...

@@ -93,6 +93,12 @@ public override object GetValue(ParseContext context)
return Inner?.GetValue(context);
}

public override void GetFoldingRanges(ICollection<FoldingRange> result)
Copy link
Contributor

Choose a reason for hiding this comment

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

Bei überschriebenen public Methoden bitte immer /// <inheritdoc />, sonst meckert die statische Code-Analyse, dass da der Kommentar fehlt

@georghinkel
Copy link
Contributor

Also im Moment sind noch fehlschlagende Tests und Merge-Konflikte, die den PR blockieren

@nhett nhett force-pushed the LSP-folding-ranges branch from 8901f69 to 6063a19 Compare January 17, 2025 11:11
Copy link
Contributor

@georghinkel georghinkel left a comment

Choose a reason for hiding this comment

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

Sieht gut aus :)

var commentRuleApplication = Comments[i];

uint endLine, endCol;
if (commentRuleApplication.Rule is MultilineCommentRule)
Copy link
Contributor

Choose a reason for hiding this comment

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

Sehe ich das richtig, dass wir hier davon ausgehen, wenn es ein Multiline Comment ist, dann gibt es genau einen, sonst könnte es mehrere geben?

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