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

Change error & hover IDE msgs to 1-based index #100

Merged
merged 10 commits into from
Sep 15, 2021
Merged

Conversation

alfredats
Copy link
Contributor

Fixes #72.

Co-authored-by: akjw [email protected]

@alfredats alfredats requested a review from anka-213 June 30, 2021 07:11
@alfredats
Copy link
Contributor Author

We just realized that we have not tackled the other aspect of the issue, which was to use the following format:

path/to/file.l4:line:col

Will continue work for now

src/L4LSP.hs Outdated Show resolved Hide resolved
@anka-213
Copy link
Collaborator

anka-213 commented Jun 30, 2021

The filename:line:column syntax depends on #96, so we remove that from this issue.

@anka-213
Copy link
Collaborator

I believe we also need to update the lexer/parser to make sure it uses 1-indexed line/column numbers. This was the case in an older version of baby-l4. I think that change would consist of simplifying the aposToPos function, so it no longer subtracts 1.

https://github.com/smucclaw/baby-l4/pull/31/files#diff-0c2bef627a0c8a5d583e6bb63f4887de2eea97aae8e259c79ed8ef0d74ff41eeR540-R541

But it would be the same style we would use for converting to 0-indexed for lsp.

@akjw
Copy link
Contributor

akjw commented Jul 2, 2021

With the amendments below, the tests are now passing:

  • change aposToPos so that SRng is 1-indexed (Lexer.x)
  • convert 1-indexed SRng to 0-indexed Range (L4LSP.hs)
  • change printPos in Error.hs to reflect 1-indexed SRng (Error.hs)
  • make tests use 0-indexed positions, since we're using the lsp datatypes, Range & Position (Test.hs)

@anka-213
Copy link
Collaborator

anka-213 commented Jul 2, 2021

It's a tradeoff if we want to use 1-index or not in the tests. We could always have mkRange convert from 1-index to 0-index, but the downside with that is that the test failures will still show the errors 0-indexed. The upside is that it's more intuitive.

On the other hand, we could modify the test-helpers so they do use 1-index internally and only convert between the two different types at the very edges (i.e. in a wrapper around the calls to external lsp library functions like getHover).

- implemented helper functions (mkRange', mkPosition) for writing the tests in a 1-indexed format for readability, and to reduce cognitive load.

Co-authored-by: akjw <[email protected]>
@alfredats
Copy link
Contributor Author

So we pondered for a bit, and went with the first option. We implement 2 helper functions (mkRange', mkPosition) that would take 1-indexed values and convert them to 0-indexed values.

Hopefully this would help decrease cognitive load when writing the tests, and also improve readability.

@alfredats alfredats requested a review from anka-213 July 13, 2021 05:37
Copy link
Collaborator

@anka-213 anka-213 left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long. Looks good to me. Thanks!

lsp-tests/Test.hs Outdated Show resolved Hide resolved
@anka-213 anka-213 marked this pull request as draft September 15, 2021 07:28
@anka-213 anka-213 marked this pull request as ready for review September 15, 2021 07:57
@mergify mergify bot merged commit 6c7456d into main Sep 15, 2021
@alfredats alfredats deleted the feat_one_indexed branch September 21, 2021 08:48
@alfredats alfredats restored the feat_one_indexed branch September 21, 2021 08:48
@alfredats alfredats deleted the feat_one_indexed branch September 21, 2021 08:49
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.

Use 1-index line and column numbers in error messages
3 participants