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

Fix range comparison #1525

Merged
merged 1 commit into from
Jun 3, 2024
Merged

Fix range comparison #1525

merged 1 commit into from
Jun 3, 2024

Conversation

jindong-zhannng
Copy link
Contributor

The start of ranges in LSP is zero-based.

I don't know is this function still in use, just fix it while reading source codes.

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

I'm not sure what this PR is supposed to fix (except for the typo in line 88 range -> to). Is there an actual issue associated with this? If not, I think it still makes sense to at least include the typo fix. Could you please at a few automated tests that assert that the function is supposed to do what it does? I don't think there currently are any tests for that function in particular.

Note that the code is designed to use < and >, since the <= and >= operators are used below (lines 93-94) to check whether a range is inside of another.

Also, in order to accept your contribution, please sign the Eclipse Contributor Agreement (ECA) with the email address that was used to create the commit.

@jindong-zhannng
Copy link
Contributor Author

I added test cases to cover all my changes. For the range comparison, here is a reference implementation from vscode: https://github.com/microsoft/vscode/blob/addd445017c9c5531b89851dc8dc9bec4f1dd0f8/src/vs/editor/common/core/range.ts#L420C16-L420C31

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Thanks for the tests!

Have you already signed the ECA with the email that was used to create the commit? The automated checking process still says it's not signed, see here.

packages/langium/src/utils/cst-utils.ts Show resolved Hide resolved
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

LGTM, thanks 👍

@msujew msujew added the polish Some feature needs improvement label Jun 3, 2024
@msujew msujew merged commit c3fd862 into eclipse-langium:main Jun 3, 2024
4 checks passed
@msujew msujew added this to the v3.1.0 milestone Jun 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
polish Some feature needs improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants