Add Goto Definition for cabal common sections by ChristophHochrainer · Pull Request #4375 · haskell/haskell-language-server

ChristophHochrainer · 2024-08-03T15:38:54Z

This PR is the start of a go to definition code action for cabal files.

fendor · 2024-08-04T09:09:41Z

@VenInf I think this PR is relevant to you as well and might be implementing some util functions you could benefit from as well. Could you review this PR?

fendor

Thanks for the PR!

This looks very nice, just a couple of tests missing, then this is good to merge :)

fendor · 2024-08-04T09:12:04Z

+    case uriToFilePath' uri of
+      Nothing ->
+        pure $ InR $ InR Null
+      Just filePath -> do


Prefer getNormalizedFilePathE which takes care of the validation.

Suggested change

case uriToFilePath' uri of

Nothing ->

pure $ InR $ InR Null

Just filePath -> do

nfp <- getNormalizedFilePathE uri

Then use fromNormalizedFilePath if you need the underlying FilePath.

fendor · 2024-08-04T09:13:35Z

@@ -1,12 +1,16 @@
-module Ide.Plugin.Cabal.Completion.CabalFields (findStanzaForColumn, findFieldSection, getOptionalSectionName, getAnnotation, getFieldName, onelineSectionArgs) where
+{-# OPTIONS_GHC -Wno-unrecognised-pragmas #-}
+module Ide.Plugin.Cabal.Completion.CabalFields (findStanzaForColumn, findFieldSection, findTextWord, findFieldLine, getOptionalSectionName, getAnnotation, getFieldName, onelineSectionArgs, getFieldEndPosition, getSectionArgEndPosition, getNameEndPosition, getFieldLineEndPosition, getFieldLSPRange) where


I am amazed stylish-haskell accepts this.

Would you mind introducing a couple of new lines to make this easier to read? E.g.

Suggested change

module Ide.Plugin.Cabal.Completion.CabalFields (findStanzaForColumn, findFieldSection, findTextWord, findFieldLine, getOptionalSectionName, getAnnotation, getFieldName, onelineSectionArgs, getFieldEndPosition, getSectionArgEndPosition, getNameEndPosition, getFieldLineEndPosition, getFieldLSPRange) where

module Ide.Plugin.Cabal.Completion.CabalFields

( findStanzaForColumn,

findFieldSection,

findTextWord,

...

) where

or something like that?

fendor · 2024-08-04T09:13:51Z

@@ -1,12 +1,16 @@
-module Ide.Plugin.Cabal.Completion.CabalFields (findStanzaForColumn, findFieldSection, getOptionalSectionName, getAnnotation, getFieldName, onelineSectionArgs) where
+{-# OPTIONS_GHC -Wno-unrecognised-pragmas #-}


Suggested change

{-# OPTIONS_GHC -Wno-unrecognised-pragmas #-}

Looks unused?

fendor · 2024-08-04T09:15:28Z

 import qualified Distribution.Fields               as Syntax
 import qualified Distribution.Parsec.Position      as Syntax
 import           Ide.Plugin.Cabal.Completion.Types
+import qualified Language.LSP.Protocol.Types       as LSPTypes


Nitpick:

Suggested change

import qualified Language.LSP.Protocol.Types as LSPTypes

import qualified Language.LSP.Protocol.Types as LSP

Other parts of the codebase use this qualified import. Perhaps we should have some kind of style guide? Hard to tell.

fendor · 2024-08-04T09:23:04Z

+-- that are not a space or column.
+
+-- This function currently only considers words inside of a @FieldLine@.


Suggested change

-- that are not a space or column.

-- This function currently only considers words inside of a @FieldLine@.

-- that are not a space or column.

--

-- This function currently only considers words inside of a @FieldLine@.

Otherwise, the haddock comment will not be attached to the function :)

VenInf · 2024-08-04T14:13:53Z

Thank you for your work!
The tools do indeed, look very handy.

The only comment I might have, is that I thought about mimicking
the approach used in the HoverDefinition.hs for the hovers/go-to definitions,
but this approach looks more practical, in some sense.

VeryMilkyJoe

Very nice feature and the code looks great, thanks!
I think once we get some tests, we're ready to merge 🥳

ChristophHochrainer · 2024-08-17T23:36:45Z

Thank you all for your feedback!

I made the requested changes and added some tests. (Thanks @fendor for the short introduction)
I also included the requested hie.yaml for the test data requested by @fendor. :)

fendor

LGTM, awesome, thank you very much!

* Add goto-definitions for cabal common sections * Add default direct cradle hie.yaml file to testdata * incorporate changes requested in haskell#4375 * add tests for cabal goto-definition

ChristophHochrainer requested a review from fendor as a code owner August 3, 2024 15:38

fendor requested a review from VeryMilkyJoe August 4, 2024 09:08

fendor requested changes Aug 4, 2024

View reviewed changes

VeryMilkyJoe requested changes Aug 14, 2024

View reviewed changes

Comment thread plugins/hls-cabal-plugin/src/Ide/Plugin/Cabal/Completion/CabalFields.hs Outdated

VenInf reviewed Aug 14, 2024

View reviewed changes

Comment thread plugins/hls-cabal-plugin/src/Ide/Plugin/Cabal.hs Outdated

VenInf mentioned this pull request Aug 14, 2024

Cabal go to module's definition #4380

Merged

4 tasks

ChristophHochrainer force-pushed the feature/cabal/import-go-to-definition branch from 62972ef to 8f164e2 Compare August 17, 2024 21:22

ChristophHochrainer added 2 commits August 17, 2024 23:22

Add goto-definitions for cabal common sections

93931fb

Add default direct cradle hie.yaml file to testdata

c2d980c

ChristophHochrainer force-pushed the feature/cabal/import-go-to-definition branch from 8f164e2 to c2d980c Compare August 17, 2024 21:24

incorporate changes requested in haskell#4375

26dcc40

add tests for cabal goto-definition

d0d0f94

ChristophHochrainer force-pushed the feature/cabal/import-go-to-definition branch from e35a426 to d0d0f94 Compare August 18, 2024 06:39

fendor approved these changes Aug 18, 2024

View reviewed changes

fendor requested review from VenInf and VeryMilkyJoe August 18, 2024 08:33

fendor changed the title ~~Add code action for cabal go to definition~~ Add Goto Definition for cabal common sections Aug 18, 2024

VeryMilkyJoe approved these changes Aug 18, 2024

View reviewed changes

fendor merged commit 6f6f75b into haskell:master Aug 18, 2024

		@@ -1,12 +1,16 @@
		module Ide.Plugin.Cabal.Completion.CabalFields (findStanzaForColumn, findFieldSection, getOptionalSectionName, getAnnotation, getFieldName, onelineSectionArgs) where
		{-# OPTIONS_GHC -Wno-unrecognised-pragmas #-}

	import qualified Language.LSP.Protocol.Types as LSPTypes
	import qualified Language.LSP.Protocol.Types as LSP

		-- that are not a space or column.

		-- This function currently only considers words inside of a @FieldLine@.

Uh oh!

Uh oh!

Conversation

ChristophHochrainer commented Aug 3, 2024

Uh oh!

fendor commented Aug 4, 2024

Uh oh!

fendor left a comment

Choose a reason for hiding this comment

Uh oh!

fendor Aug 4, 2024

Choose a reason for hiding this comment

Uh oh!

fendor Aug 4, 2024

Choose a reason for hiding this comment

Uh oh!

fendor Aug 4, 2024

Choose a reason for hiding this comment

Uh oh!

fendor Aug 4, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fendor Aug 4, 2024

Choose a reason for hiding this comment

Uh oh!

VenInf commented Aug 4, 2024

Uh oh!

VeryMilkyJoe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ChristophHochrainer commented Aug 17, 2024

Uh oh!

fendor left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants