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

Cabal file completions #3268

Merged
merged 2 commits into from
Aug 3, 2023
Merged

Conversation

VeryMilkyJoe
Copy link
Collaborator

@VeryMilkyJoe VeryMilkyJoe commented Oct 8, 2022

Add completion suggestions for cabal files.

Related to #3664, by suggestion of #2940.

Current state:

Completions for keywords

  • sensitive to stanzas

Value completion for different kinds of values

We implement completion according to the cabal package description.

  • Various simple static values
  • Name field (based on cabal file name)
  • Licenses
  • Files
  • Directories
  • Exposed modules
  • Other modules

Snippet completion

  • Snippet for common keywords in cabal file
  • Snippets for stanzas and their required fields
  • Snippet for common-warnings

Examples

Keyword completion inside a stanza:
image

Value completion for existing keyword:
image

Snippet for library stanza
image

@VeryMilkyJoe VeryMilkyJoe self-assigned this Oct 8, 2022
@VeryMilkyJoe VeryMilkyJoe force-pushed the cabal-completions branch 3 times, most recently from 97a0cf6 to 9bf0eaf Compare October 9, 2022 11:24
@VeryMilkyJoe VeryMilkyJoe changed the title Draft: Cabal keyword completions Draft: Cabal file completions Oct 9, 2022
@VeryMilkyJoe VeryMilkyJoe marked this pull request as draft October 13, 2022 09:52
@VeryMilkyJoe VeryMilkyJoe force-pushed the cabal-completions branch 5 times, most recently from 2038fdb to 182189b Compare May 31, 2023 09:09
@VeryMilkyJoe VeryMilkyJoe force-pushed the cabal-completions branch 2 times, most recently from 0ec6fe4 to c46ba7d Compare June 5, 2023 16:25
@VeryMilkyJoe VeryMilkyJoe force-pushed the cabal-completions branch 5 times, most recently from 5c5a6a3 to 7a6d82b Compare June 16, 2023 15:14
@VeryMilkyJoe VeryMilkyJoe marked this pull request as ready for review June 16, 2023 15:20
@VeryMilkyJoe VeryMilkyJoe changed the title Draft: Cabal file completions Cabal file completions Jun 16, 2023
@VeryMilkyJoe VeryMilkyJoe force-pushed the cabal-completions branch 2 times, most recently from 70e7d3d to ada3138 Compare June 19, 2023 10:24
@fendor fendor requested a review from michaelpj June 19, 2023 10:26
@VeryMilkyJoe VeryMilkyJoe force-pushed the cabal-completions branch 4 times, most recently from 0027a1c to 9c3821f Compare June 19, 2023 15:36
Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

Generally look good! I think my main high-level worry is that this might not deal well with large files because it looks like it's doing a decent amount of stuff that's linear in the number of lines. Yes, cabal files tend to be short, but they could be long, e.g. imagine if we did actually merge all the HLS packages into a single package with many libraries!

I had two thoughts about fixing this:

  1. Use the Rope type that the VFS uses for file contents, which has good performance for line-based operations
  2. Actually parse the file into some kind of structure which we can then use for efficiently looking up where a position is later. This would be more work, I'm not sure if we could reuse the cabal parser or whether we'd need to do something else, but in principle you could look at the source spans for syntax elements and try to descend into the closest piece of syntax while hopefully doing a number of operations proportional to the depth of the syntax tree instead.

@VeryMilkyJoe
Copy link
Collaborator Author

  1. Use the Rope type that the VFS uses for file contents, which has good performance for line-based operations

Thank you! I will include this.

  1. Actually parse the file into some kind of structure which we can then use for efficiently looking up where a position is later. This would be more work, I'm not sure if we could reuse the cabal parser or whether we'd need to do something else, but in principle you could look at the source spans for syntax elements and try to descend into the closest piece of syntax while hopefully doing a number of operations proportional to the depth of the syntax tree instead.

I have thought about this, since it would probably make a lot of the cabal plugin's features more neat in the long run.
As far as I can tell there is no AST representation of cabal files, which is what we would want, I only found the GenericPackageDescription which does not contain source spans and neither does the cabal parser. It seems like there is no intermediate representation used anywhere (https://hackage.haskell.org/package/Cabal-syntax-3.10.1.0/docs/Distribution-PackageDescription-Parsec.html#v:parseGenericPackageDescription).
It might be nice to implement our own representation but I would argue that this is out of scope for this PR.

Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

First review round! 🎊

I think this is in a good shape, couple of tests, couple of docs and then we can likely merge it, imo.

Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

Ran out of time so I just commented on the file completions.

@VeryMilkyJoe VeryMilkyJoe force-pushed the cabal-completions branch 6 times, most recently from 1abd3c5 to 7e998da Compare July 10, 2023 08:34
Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

LGTM, last review round, then we will merge this.

Includes:
* completions for keywords, sensitive to stanzas
* value completions for constant values, files, directories and exposed modules
* completion of snippets for different stanzas and the required basic fields
Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

LGTM, let's get this merged and get some user feedback.

@fendor fendor enabled auto-merge (rebase) August 3, 2023 09:23
@fendor fendor merged commit 202295b into haskell:master Aug 3, 2023
42 checks passed
@fendor fendor mentioned this pull request Aug 8, 2023
19 tasks
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.

3 participants