Draft: Change API to emit multiple ComponentOptions if available #304

fendor · 2021-08-23T09:10:39Z

Introduce API changes necessary for #301

This PR can be merged before #301, and released, then HLS can be updated for the new API.

fendor · 2021-08-23T09:11:03Z

jneira

horror vacui 👍

fendor · 2021-09-14T14:41:32Z

What are the semantics of the provided NonEmpty? Do we require that the component the file path is part of has been loaded and is the first element of the NonEmpty? How can we communicate that we can not load a component because one of our dependencies has failed to build?
I propose, mainly for the sake of argument, a new data type instead of NonEmpty:

data LoadResult = 
  { loadComponent :: Maybe ComponentOptions,
  , componentDependencies :: Maybe (NonEmpty ComponentOptions)
  }

intended semantics: it is possible to extract component dependencies and load these immediately, but we were unable to load the component the file target is part of.
No guarantee for the order of dependencies. They may not exist, but if they do exist, there must exist at least. This type is isomorphic to just [ComponentOptions], but maybe expresses the intent better.

Opinions?

jneira · 2021-09-15T12:38:45Z

Opinions?

i like the precision of the new data type and it can be extended if we find out other nuances

fendor · 2021-10-13T08:13:44Z

src/HIE/Bios/Types.hs

+type LoadResult = LoadResult' ComponentOptions
+
+data LoadResult' a = LoadResult
+  { loadResultComponent :: Maybe a
+  -- ^ Component options for the FilePath that produced this 'LoadResult'.
+  -- See 'CradleAction.runCradle' for information on how to produce a 'LoadResult'.
+  --
+  -- This field can be 'Nothing' to indicate that loading partially failed.
+  , loadResultDependencies :: [a]
+  -- ^ Direct or indirect dependencies from the component from above.
+  -- Indirect means that it is not required that 'ComponentOptions' in this list
+  -- are required dependencies of 'loadResultComponent'. It is specifically allowed
+  -- to list 'ComponentOptions' that have no relation with 'loadResultComponent'.
+  --
+  -- Example:
+  --
+  -- Assume we load an executable component, then its options must be located
+  -- in 'loadResultComponent', additionally it is possible to list other
+  -- executable component's options in 'loadResultDependencies'.
+  } deriving (Eq, Ord, Show, Functor, Foldable, Traversable)


I honestly don't really like this naming. Signatures be like: CradleLoadResult LoadResult. Seems really redundant

fendor · 2021-10-13T09:22:31Z

src/HIE/Bios/Types.hs

+pattern Main :: a -> LoadResult' a
+pattern Main opts <- (loadResultComponent -> Just opts)


We can have more of these such as:

Deps :: [a] -> LoadResult' a MainAndDeps :: a -> [a] -> LoadResult' a

for typesafe pattern matching.

Well, if we need patterns, we might just introduce a more elaborated datatype

jneira · 2021-12-15T07:31:05Z

@fendor hi! any chance to get this merged to make progress in enable rename plugin in hls?

fendor · 2021-12-15T08:08:39Z

It is right now still kind of a WIP. Do you have opinions on naming?

fendor · 2021-12-15T08:11:13Z

src/HIE/Bios/Types.hs

+  -- Indirect means that it is not required that 'ComponentOptions' in this list
+  -- are required dependencies of 'loadResultComponent'. It is specifically allowed


The sentence makes no sense, what I mean is, that items here are either local dependencies in some form, or completely independent but can be loaded, too.
It might still be important to have some kind of order on them.

jneira · 2021-12-15T08:28:50Z

src/HIE/Bios/Types.hs

+
+-- | Record for expressing successful loading.
+-- Can express partial success.
+data LoadResult' a = LoadResult


What about somethin like

data ComponentInfo = ComponentInfo { componentOptions :: Maybe a componentDependencies: [a] }

It would be generic enough to allow add more component information afterwards

Hm, maybe we can drop the prefix somehow to be less "Component"-y? However, since the record field prefix will probably still be required, probably pointless.

yeah, Info sounds good too, maybe too generic but whatever

michaelpj · 2022-08-16T10:23:05Z

What's the status of this?

fendor · 2022-08-16T10:32:25Z

It is on hold because it is semi-blocked on haskell/cabal#7500 to get reliably the build-info location.

Technically, it is not blocked, the API could be changed without cabal support, but then we have a slightly harder time to test the implementation. Also, if there is no cabal or stack support for it, I see barely an incentive to implement it, currently.

fendor · 2023-11-06T15:05:02Z

Superseded by #409

fendor requested a review from jneira August 23, 2021 09:10

jneira approved these changes Aug 23, 2021

View reviewed changes

fendor force-pushed the feature/non-empty branch from eab203d to 5ba89dd Compare August 23, 2021 14:15

OliverMadine mentioned this pull request Sep 14, 2021

Renaming multi component support haskell/haskell-language-server#2193

Open

fendor force-pushed the feature/non-empty branch 4 times, most recently from 73b4f97 to 556d09d Compare October 13, 2021 08:51

fendor commented Oct 13, 2021

View reviewed changes

fendor force-pushed the feature/non-empty branch from 556d09d to 8fd241a Compare October 13, 2021 09:35

fendor changed the title ~~Change API to emit NonEmpty ComponentOptions~~ Draft: Change API to emit NonEmpty ComponentOptions Dec 15, 2021

fendor commented Dec 15, 2021

View reviewed changes

fendor added 3 commits December 15, 2021 09:27

Change API to emit NonEmpty ComponentOptions

31149cd

Remove unused type

3d0bc67

Add explicit type for LoadResults

c3ba11b

fendor force-pushed the feature/non-empty branch from 8fd241a to c3ba11b Compare December 15, 2021 08:27

jneira reviewed Dec 15, 2021

View reviewed changes

Merge branch 'master' into feature/non-empty

2861f10

OliverMadine mentioned this pull request Jan 18, 2022

Enable rename plugin haskell/haskell-language-server#2598

Closed

fendor changed the title ~~Draft: Change API to emit NonEmpty ComponentOptions~~ Draft: Change API to emit multiple ComponentOptions if available May 3, 2022

fendor closed this Nov 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft: Change API to emit multiple ComponentOptions if available #304

Draft: Change API to emit multiple ComponentOptions if available #304

fendor commented Aug 23, 2021 •

edited

fendor commented Aug 23, 2021

jneira left a comment

fendor commented Sep 14, 2021

jneira commented Sep 15, 2021

fendor Oct 13, 2021

fendor Oct 13, 2021

fendor Oct 27, 2021

jneira commented Dec 15, 2021

fendor commented Dec 15, 2021

fendor Dec 15, 2021

jneira Dec 15, 2021

fendor Dec 15, 2021

jneira Dec 15, 2021 •

edited

michaelpj commented Aug 16, 2022

fendor commented Aug 16, 2022

fendor commented Nov 6, 2023

		pattern Main :: a -> LoadResult' a
		pattern Main opts <- (loadResultComponent -> Just opts)

		-- Indirect means that it is not required that 'ComponentOptions' in this list
		-- are required dependencies of 'loadResultComponent'. It is specifically allowed

Draft: Change API to emit multiple ComponentOptions if available #304

Draft: Change API to emit multiple ComponentOptions if available #304

Conversation

fendor commented Aug 23, 2021 • edited

fendor commented Aug 23, 2021

jneira left a comment

Choose a reason for hiding this comment

fendor commented Sep 14, 2021

jneira commented Sep 15, 2021

fendor Oct 13, 2021

Choose a reason for hiding this comment

fendor Oct 13, 2021

Choose a reason for hiding this comment

fendor Oct 27, 2021

Choose a reason for hiding this comment

jneira commented Dec 15, 2021

fendor commented Dec 15, 2021

fendor Dec 15, 2021

Choose a reason for hiding this comment

jneira Dec 15, 2021

Choose a reason for hiding this comment

fendor Dec 15, 2021

Choose a reason for hiding this comment

jneira Dec 15, 2021 • edited

Choose a reason for hiding this comment

michaelpj commented Aug 16, 2022

fendor commented Aug 16, 2022

fendor commented Nov 6, 2023

fendor commented Aug 23, 2021 •

edited

jneira Dec 15, 2021 •

edited