Lots of cleanups and a minor bug fix #1096

Merged
merged 8 commits into from Nov 6, 2012

Conversation

Projects
None yet
5 participants
Contributor

Peaker commented Nov 4, 2012

Fix a minor bug (usage print printed "cmd cabal" rather than "cabal cmd").
Also lots more cleanups (the ones I deemed positive from the huge hlint output)

Great. Can you also make upgrade a hidden command? Just change

,upgradeCommand         `commandAddAction` upgradeAction

to

,hiddenCommand $
 upgradeCommand         `commandAddAction` upgradeAction

Oh, I see that you did that in a separate commit.

Member

23Skidoo commented Nov 5, 2012

OK, everything looks good to me. @tibbe, can you merge this in?

@tibbe tibbe and 2 others commented on an outdated diff Nov 5, 2012

cabal-install/Distribution/Client/Config.hs
unlines (map (showPWarning configFile) ws)
return conf
Just (ParseFailed err) -> do
let (line, msg) = locatedErrorMsg err
warn verbosity $
"Error parsing config file " ++ configFile
- ++ maybe "" (\n -> ":" ++ show n) line ++ ":\n" ++ msg
- warn verbosity $ "Using default configuration."
+ ++ maybe "" ((':' :) . show) line ++ ":\n" ++ msg
@tibbe

tibbe Nov 5, 2012

Owner

This is one the cases where eta-reducing makes thinks less readable in my opinion. Before it was visually clear where the ":" went, now you have to think twice to understand that.

@Peaker

Peaker Nov 5, 2012

Contributor

I am just as used to point-free as I am to point-ful. For me it is just as clear, except the point-free one produces less hlint warnings :)

@simonmar

simonmar Nov 5, 2012

Owner

On 05/11/12 19:36, Eyal Lotem wrote:

In cabal-install/Distribution/Client/Config.hs:

     unlines (map (showPWarning configFile) ws)
   return conf
 Just (ParseFailed err) -> do
   let (line, msg) = locatedErrorMsg err
   warn verbosity $
       "Error parsing config file " ++ configFile
  •    ++ maybe "" (\n -> ":" ++ show n) line ++ ":\n" ++ msg
    
  •  warn verbosity $ "Using default configuration."
    
  •    ++ maybe "" ((':' :) . show) line ++ ":\n" ++ msg
    

I am just as used to point-free as I am to point-ful. For me it is just
as clear, except the point-free one produces less hlint warnings :)

Maybe hlint is complaining about the use of ++ instead of :, rather than
the pointful style? I definitely agree with Johan, the point-free
version is less readable, because it involves doing an extra reduction
step in your head.

@Peaker

Peaker Nov 5, 2012

Contributor

Reverted to the lambda format, and changed to use ':' (which is indeed the source of the hlint warning).

That said, I disagree about the point-free, as the reduction step is only necessary if you're used to reading lambdas more than reading point-free style.

In my own code I generally use point-free more than lambdas, so for me the necessary reduction step is the other way around...

@tibbe tibbe and 1 other commented on an outdated diff Nov 5, 2012

...all/Distribution/Client/Dependency/Modular/Builder.hs
@@ -44,7 +44,7 @@ extendOpen :: QPN -> [OpenGoal] -> BuildState -> BuildState
extendOpen qpn' gs s@(BS { rdeps = gs', open = o' }) = go gs' o' gs
where
go g o [] = s { rdeps = g, open = o }
- go g o (ng@(OpenGoal (Flagged _ _ _ _) _gr) : ngs) = go g (cons ng () o) ngs
+ go g o (ng@(OpenGoal (Flagged {}) _gr) : ngs) = go g (cons ng () o) ngs
@tibbe

tibbe Nov 5, 2012

Owner

Wether to use {} or repeated _ is a matter of style, so please don't change them all.

@Peaker

Peaker Nov 5, 2012

Contributor

As with above, I wanted cleaner hlint (especially as I would like to be able to happily use hlint on my own developments in cabal itself). In any case, is there a benefit to listing the number of fields in the constructor here?

@tibbe

tibbe Nov 6, 2012

Owner

Just so we're on the same page: being hlint clean is a non-goal for Cabal. Any changes that are made just to make hlint (which is really just an embodiment of Neil's coding style) happy will introduce lots of noise in development. Don't get me wrong, hlint is an excellent source of suggestions for changes (which the many good changes in your patches show), but they're not all good.

To be specific, replacing all Foo _ _ pattern matches in Cabal with Foo {} is nothing I want to do.

@tibbe tibbe and 1 other commented on an outdated diff Nov 5, 2012

cabal-install/Distribution/Client/HttpUtils.hs
@@ -1,4 +1,5 @@
{-# OPTIONS -cpp #-}
+-- TODO: Did old, supported GHC's not support the "LANGUAGE CPP" pragma?
@tibbe

tibbe Nov 5, 2012

Owner

Cabal tries to support as many old GHCs as we can. At least back to 6.12.3.

@23Skidoo

23Skidoo Nov 5, 2012

Member

{-# LANGUAGE CPP #-} is used in other files, though.

$ ack -l 'LANGUAGE CPP' | wc -l
18

@tibbe tibbe commented on an outdated diff Nov 5, 2012

cabal-install/Distribution/Client/Index.hs
@@ -214,5 +214,5 @@ listBuildTreeRefs verbosity path = do
checkIndexExists :: FilePath -> IO ()
checkIndexExists path = do
indexExists <- doesFileExist path
- when (not indexExists) $ do
+ unless indexExists .
@tibbe

tibbe Nov 5, 2012

Owner

While to . happens to be legal here, it's more consistent if we use $, like we do everywhere else.

@tibbe tibbe and 1 other commented on an outdated diff Nov 5, 2012

cabal-install/Distribution/Client/Init.hs
@@ -164,7 +162,7 @@ getPackageName flags = do
-- if possible.
getVersion :: InitFlags -> IO InitFlags
getVersion flags = do
- let v = Just $ Version { versionBranch = [0,1,0,0], versionTags = [] }
+ let v = Just Version { versionBranch = [0,1,0,0], versionTags = [] }
@tibbe

tibbe Nov 5, 2012

Owner

While record notation binds tighter than function application, it's nothing that's immediately clear, so I prefer to have the extra $ here.

@Peaker

Peaker Nov 5, 2012

Contributor

This isn't really ambiguous though (under the assumption that it builds successfully).

@tibbe tibbe commented on the diff Nov 5, 2012

cabal-install/Distribution/Client/Init.hs
[ fieldS "main-is" NoFlag (Just ".hs or .lhs file containing the Main module.") True
, generateBuildInfo Executable c
])
- Flag Library -> text "\nlibrary" $$ (nest 2 $ vcat
+ Flag Library -> text "\nlibrary" $$ nest 2 (vcat
@tibbe

tibbe Nov 5, 2012

Owner

Be careful when you remove parenthesis in pretty-printing code. The expression without the parenthesis might be legal, but might mean something slightly different.

@Peaker

Peaker Nov 5, 2012

Contributor

This is based on an hlint "redundant $" warning that wants to translate:

f (g param $ some stuff)

to:

f g param (some stuff)

So the parens are just replacing the $

@tibbe tibbe and 1 other commented on an outdated diff Nov 5, 2012

cabal-install/Distribution/Client/Init/Heuristics.hs
@@ -193,7 +193,7 @@ nameAndMail str
-- split string at given character, and remove whitespaces
splitString :: Char -> String -> [String]
-splitString sep str = go str where
+splitString sep = go where
@tibbe

tibbe Nov 5, 2012

Owner

FYI, GHC treats

f x = ...

different than

f = \x -> ...

for optimization purposes. In particular, it only inlines functions that have been applied to all it's syntactically visible arguments.

@Peaker

Peaker Nov 5, 2012

Contributor

So this change should cause more code to be inlined?

The reason I believe in eta-reducing code like this is because otherwise, "str" is in scope in the context of "go", which is most likely never supposed to use "str" but only its reference to some tail of "str". So I think it is preferable to avoid putting irrelevant things of the correct type in lexical scope to make sure they're not used.

@tibbe tibbe and 1 other commented on an outdated diff Nov 5, 2012

cabal-install/Distribution/Client/InstallSymlink.hs
@@ -1,5 +1,6 @@
{-# OPTIONS -cpp #-}
-- OPTIONS required for ghc-6.4.x compat, and must appear first
+-- TODO: ghc 6.12.1 is more than 3 years old. Can this be dropped?
@Peaker

Peaker Nov 5, 2012

Contributor

Can you expand on this? The HACKING file says only 3 year old GHC's are supported, so I think it would be nice to clean up all the CPP pragmas to be the same LANGUAGE CPP rather than OPTIONS -cpp.

Is there a policy to support older ghc's too?

@tibbe

tibbe Nov 6, 2012

Owner

We recently fixed cabal-install to work with GHC 6.12.3 on requests by users, so we don't try to break old versions when the benefit is very small, as in this case.

@tibbe tibbe commented on the diff Nov 5, 2012

cabal-install/Distribution/Client/Sandbox.hs
-- | Entry point for the 'cabal dump-pkgenv' command.
dumpPackageEnvironment :: Verbosity -> SandboxFlags -> IO ()
dumpPackageEnvironment verbosity sandboxFlags = do
- sandboxDir <- getSandboxLocation verbosity sandboxFlags
- pkgEnvDir <- getCurrentDirectory
- pkgEnv <- tryLoadPackageEnvironment verbosity sandboxDir pkgEnvDir
+ (_sandboxDir, pkgEnvDir, pkgEnv) <- getSandboxInfo verbosity sandboxFlags
@tibbe

tibbe Nov 5, 2012

Owner

The fact that different call sites ignore some of the values returned by getSandboxInfo suggests that they should only call those functions needed to produce the values actually used.

@23Skidoo

23Skidoo Nov 5, 2012

Member

sandboxDir and pkgEnvDir are needed as arguments to tryLoadPackageEnvironment.

@tibbe

tibbe Nov 6, 2012

Owner

I'm confused then. _sandboxDir has a leading underscore in its name, which indicates that it's unused.

More generally, I don't think

helper = do
   a <- f
   b <- g
   c <- h

test = do
    (a, b, c) <- helper

is much of an improvement over

test = do
   a <- f
   b <- g
   c <- h

If one of the return values aren't use, I think it's strictly worse because we perform unnecessary computation. If we want to make such a refactoring, we should at least raise the abstraction level by identifying some coherent set of properties of sandboxes and create e.g. a SandboxConfig data type to embody them.

@23Skidoo

23Skidoo Nov 6, 2012

Member

It's more like

helper = do
   a <- f
   b <- g
   c <- h a b

I agree that it's not obviously an improvement, but I don't really care since this code is going to be removed anyway.

@tibbe tibbe commented on the diff Nov 5, 2012

cabal-install/Distribution/Client/Setup.hs
@@ -1301,7 +1297,7 @@ sandboxInitCommand = CommandUI {
commandName = "sandbox-init",
commandSynopsis = "Initialise a fresh sandbox",
commandDescription = Nothing,
- commandUsage = \pname -> usageFlags pname "sandbox-init",
+ commandUsage = usageFlags "sandbox-init",
@tibbe

tibbe Nov 5, 2012

Owner

Is this equivalent? It seems like the two string arguments to usageFlags got swapped around.

@23Skidoo

23Skidoo Nov 5, 2012

Member

It's a bug fix.

$ cabal help sandbox-init
Usage: sandbox-init cabal [FLAGS]
@tibbe

tibbe Nov 6, 2012

Owner

Good catch. In the future, it's better to separate behavior-preserving and behavior-modifying commits, as it's hard to review something where both what is computed and how it's computed is changed at the same time, especially when the change is large.

@tibbe tibbe commented on the diff Nov 5, 2012

cabal-install/Main.hs
@@ -465,22 +468,18 @@ updateAction verbosityFlag extraArgs globalFlags = do
upgradeAction :: (ConfigFlags, ConfigExFlags, InstallFlags, HaddockFlags)
-> [String] -> GlobalFlags -> IO ()
-upgradeAction (configFlags, configExFlags, installFlags, haddockFlags)
@tibbe

tibbe Nov 5, 2012

Owner

Is this code no longer used?

@23Skidoo

23Skidoo Nov 5, 2012

Member

The upgrade command is disabled. upgradeAction calls upgrade, which just calls die.

@tibbe tibbe commented on an outdated diff Nov 5, 2012

cabal-install/Main.hs
@@ -529,7 +528,7 @@ uploadAction uploadFlags extraArgs globalFlags = do
checkAction :: Flag Verbosity -> [String] -> GlobalFlags -> IO ()
checkAction verbosityFlag extraArgs _globalFlags = do
- unless (null extraArgs) $ do
+ unless (null extraArgs) .
@tibbe

tibbe Nov 5, 2012

Owner

See other comment about replacing . with $.

Owner

tibbe commented Nov 5, 2012

The majority of the changes look good (thanks for cleaning up)! I left some comments, please address those. Also don't forget to compile the code and run the tests:

cd Cabal
cabal configure --enable-tests && cabal build && cabal test
cd ../cabal-install
cabal configure --package-db=../Cabal/dist/package.conf.inplace && cabal build
Contributor

Peaker commented Nov 5, 2012

Re-pushed in light of the comments. Hope I didn't miss something.

Owner

tibbe commented Nov 6, 2012

Thanks for your patience during the code review. I've merged your changes.

@tibbe tibbe merged commit adf7c7a into haskell:master Nov 6, 2012

I think this is an unnecessary change.

Sorry, but I think the old version is clearer.

Once again, I see no benefit in this change.

Contributor

kosmikus commented on 74ffcfd Nov 8, 2012

I disagree with some of the changes here, in particular removing of parentheses from type signatures that serve documentation about how a function is supposed to be used, and the introduction of combinators such as first to replace perfectly valid lambda abstractions.

I hope we're not adopting a policy to blindly do everything that hlint suggests. A tool such as hlint has no contextual information, but code is written with such information in mind. I consider myself to be maintaining the modular dependency solver, and would like to keep the code in there to be written in such a way that makes it easiest for me to understand what's going on. I would just revert these changes, but want to ensure in advance that this will not result in a sequence of changes back and forth.

Member

23Skidoo replied Nov 8, 2012

@kosmikus I don't think anyone will insist on changes you disagree with.

Owner

tibbe replied Nov 8, 2012

I disagree with some of the changes here, in particular removing of parentheses from type signatures that serve documentation about how a function is supposed to be used, and the introduction of combinators such as first to replace perfectly valid lambda abstractions.

Feel free to revert the changes you don't agree with.

I hope we're not adopting a policy to blindly do everything that hlint suggests. A tool such as hlint has no contextual information, but code is written with such information in mind.

Definitely not and I said as much in the code review.

23Skidoo referenced this pull request May 5, 2017

Merged

Partially linted #4495

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment