Simple refactoring #5926
Simple refactoring #5926
Conversation
theobat
force-pushed
the
simple-refactoring
branch
from
48db3c1 to
ceab810
Compare
…es will be component addressed and not package adressed (to avoid circular imports in : Dependency <- Component <- Package )
theobat
force-pushed
the
simple-refactoring
branch
2 times, most recently
from
32dc739 to
d71a79c
Compare
|
@mpilgrem this one is ready to be merged it's a cleaning worth the merge no matter what. (unused functions etc) |
src/Stack/Build/ConstructPlan.hs
Outdated
| @@ -52,6 +52,7 @@ import System.Environment (lookupEnv) | |||
| import System.IO (putStrLn) | |||
| import RIO.PrettyPrint | |||
| import RIO.Process (findExecutable, HasProcessContext (..)) | |||
| import Stack.Types.Dependency (DepValue(DepValue), DepType (AsLibrary)) | |||
There was a problem hiding this comment.
Please could this import line adopt the same format as used elsewhere in the module.
| {-# LANGUAGE RecordWildCards #-} | ||
| {-# LANGUAGE NamedFieldPuns #-} |
There was a problem hiding this comment.
I did not follow what prompted this change.
There was a problem hiding this comment.
This change was motivated by a simplified tracking of what is used from the Package type (as I intend to change this type to have a more cabal-like structure as stated in the issue, and thus I have to know what is used where). It's not strictly necessary, but I believe RecordWildCards is a bad and unnecessary extension (especially in this case, when 10+ fields are used it's questionable but obviously unseful).
| testconfig = config | ||
| { packageConfigEnableTests = True | ||
| , packageConfigEnableBenchmarks = False | ||
| } | ||
| benchconfig = config | ||
| { packageConfigEnableTests = False | ||
| , packageConfigEnableBenchmarks = True | ||
| } |
There was a problem hiding this comment.
For my benefit, could you explain further why these can be safely deleted? I am not across the architectural considerations and would appreciate some help in understanding them.
There was a problem hiding this comment.
Honestly I don't know why it existed in the first place, as it's never been used as far as I know.
There was a problem hiding this comment.
The fields have been there for a very long time (7+ years) and I don't know at which point they stopped being used.
The reason why it's important to remove them is that, we should have dependencies tracked at the component level for mixins/backpack support, not at the LocalPackage/Package level.
src/Stack/Package.hs
Outdated
| @@ -77,6 +77,7 @@ import System.IO.Error | |||
| import RIO.Process | |||
| import RIO.PrettyPrint | |||
| import qualified RIO.PrettyPrint as PP (Style (Module)) | |||
| import Stack.Types.Dependency (DepValue(..), DepType (..)) | |||
There was a problem hiding this comment.
Please could this import line adopt the same format as used elsewhere in the module.
src/Stack/Types/Dependency.hs
Outdated
| @@ -0,0 +1,29 @@ | |||
| {-# LANGUAGE NoImplicitPrelude #-} | |||
|
|
|||
| module Stack.Types.Dependency where | |||
There was a problem hiding this comment.
Stack's other modules tend to have express export lists.
src/Stack/Types/Dependency.hs
Outdated
| { dvVersionRange :: !VersionRange | ||
| , dvType :: !DepType | ||
| } | ||
| deriving (Show,Typeable) |
There was a problem hiding this comment.
Please could there be a space after the comma? That would be consistent within this module and with other Stack modules. (I appreciate that consistency was missing from the code you moved!)
|
@theobat, thanks for the explanations. |
This is a preparation work for #5920. It really is a simple review of unused fields and I had to move all the Dependency related types to a dedicated module because having them in Package made circular imports when moving dependencies to Components.
@mpilgrem