Hi, David Roundy <[EMAIL PROTECTED]> writes: > I wonder if it might be a good idea to not export hashedDir? It > wouldn't gain us much in the way of safety, but I don't know that we > need hashedDir outside Repository.Cache, and generally it seems a good > idea to avoid exporting functions that aren't needed: it makes the > export list more useful to folks reading the code, and it also > prevents possible abuses. In particular, it seems like a bad idea for > code outside of Repository.Cache to ever read or write directly in the > hashed directories--the whole point of this module is to govern the > interaction with hashed files, for instance, so that we'll > automatically download hashed files that we might happen to be > missing, properly using a global cache (if configured) and all that. True, it might be a little tricky to avoid the export, as the function has a couple uses in HashedIO and HashedRepo -- moreover, I see no obvious higher-level replacement that could be exported to those two modules.
Maybe: describeHashFile :: HashedDir -> String -> String describeHashFile subdir hash = hash ++ " in subdir " ++ hashedDir subdir that could be generally used by debugMessage (it would still be mostly useful inside Cache, it seems) and a hashedFilePath :: CacheLoc -> HashedDir -> String -> String hashedFilePath (Cache Directory _ d) hd h = d ++ "/" ++ hashedDir hd ++ "/" ++ h hashedFilePath (Cache Repo _ r) hd h = r ++ "/" ++ darcsdir "/" ++ hashedDir hd ++ "/" ++ h which should be able to replace those "fn" locals duplicated a few times in Cache.lhs (quick grep reveals 4 apparently identical instances). If we want to push for removing the export, we might still need to drop some debugging output and/or shuffle the code around a little, though. I'd probably defer that to bundle with some possible future export refactoring in the neighbouring modules. Probably haddocking functions as "appropriate only for Foo and Bar modules" might be an acceptable interim solution that doesn't involve code shuffles. Obviously, it won't be machine-checked, but sometimes you can't have your cake and eat it too. (Yes, a way to restrict exports in the Haskell module system would probably help us here. I don't think that's possible though?) (And as a side-note #1, I'd probably add, that reducing export lists and increasing module separation is, I believe, a good way to improve overall code quality and comprehensibility: small, well-defined interfaces play a key role in, at least my, ability to understanding code. Moreover, such code separation tends to reduce impact of changes to smaller areas of code, which is good as well.) As a side-note #2, generally, the path handling seems to be a little fragile overall, looking at all those examples of constructing paths. I'd probably prefer an unified way to get paths into working and to repository, although the access methods seem to be a mix of "assume our CWD is our repo", "wrap in withCurrentDirectory, extract path from Repository" and possibly some others. A cleanup on that front sounds like a non-trivial undertaking for sure, but would improve code safety (and clarity), I believe. Maybe that's something the RepoPath work has started to address, I haven't looked in much detail there (but the quick look struck me as being a little hairy and that it might be possible to replace most of the guts with System.FilePath -- something Eric has proposed, along a safe-ish migration path, although laborious at best...). Probably just a little someday-ish sigh. > >> hunk ./src/Darcs/Repository/Cache.lhs 118 >> cacheHasIt (Cache t Writable d:cs) = do ex <- doesFileExist (fn t >> d) >> if ex then return True >> else cacheHasIt cs >> - fn Directory d = d ++ "/" ++ subdir ++ "/" ++ f >> - fn Repo r = r ++ "/"++darcsdir++"/" ++ subdir ++ "/" ++ f >> + fn Directory d = d ++ "/" ++ (hashedDir subdir) ++ "/" ++ f >> + fn Repo r = r ++ "/"++darcsdir++"/" ++ (hashedDir subdir) ++ "/" >> ++ f > > Note that the parens around hashedDir subdir aren't needed. This > isn't something that needs fixing, but in general I prefer to avoid > unnecessary parens. It makes the code a bit more concise, and for me > is easier to parse (partly because I know the precedence rules... but > it's definitely worth learning the precedence rules, particularly for > : and ++). Right, if you'd ask me, I'd be able to tell that application takes precedence here, I just sometimes have inclinations to add a little more parens than needed, by default. I can try to keep them down for the future. :) Yours, Petr (doing one too many things at once, hopefully the post still makes some sense). -- Peter Rockai | me()mornfall!net | prockai()redhat!com http://blog.mornfall.net | http://web.mornfall.net "In My Egotistical Opinion, most people's C programs should be indented six feet downward and covered with dirt." -- Blair P. Houghton on the subject of C program indentation _______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
