2 patches for repository http://darcs.net:

Fri Feb 24 23:46:33 EST 2012  Will Langstroth <will@langstroth.com>
  * Minor formatting of Darcs.Commands.Dist

Sun Mar 11 15:28:50 EDT 2012  Will Langstroth <will@langstroth.com>
  * Style Darcs.Utils
  More style guide changes, and Haddock-ready type declarations.

Attachments

• minor-formatting-of-darcs_commands_dist.dpatch
• patch-preview.txt
• unnamed

Hi Will,

Thanks for another cleanup patch!

> hunk ./src/Darcs/Commands/Dist.hs 125
> -distCmd :: [DarcsFlag] -> [String] -> IO ()
>
+--------------------------------------------------------------------------------
> +distCmd :: [DarcsFlag]
> +        -> [String]
> +        -> IO ()

I'm not sure about two parts of these patches:

the "-----" lines seem unecessary to me and are the sort of thing that'll be
added once in these clean-up patches but then never by anyone adding new
functions. I would leave them out, personally. What purpose do they serve?

Secondly, I think the wrapping of the type-signature is only necessary if
the
length is too great, or if you wish to add end-of-line comments to
individual
parameter types. Otherwise we're increasing the number of lines for no real
benefit (that I can see).

The reason I'm mentioning these is that I've been slightly pushing recently
for
Darcs to adopt Tibbe's style guide[1], which I find to be a nice "go to" for
style questions - it's not really officially agreed on though. But it'd make
sense if our clean up patches are following the same guidelines!

Thanks for you effort in helping to clean up Darcs :-)!

Cheers,
Owen.

Attachments

• unnamed

>
> Thanks for another cleanup patch!
>

You're welcome!


>
> > hunk ./src/Darcs/Commands/Dist.hs 125
> > -distCmd :: [DarcsFlag] -> [String] -> IO ()
> >
>
> +--------------------------------------------------------------------------------
> > +distCmd :: [DarcsFlag]
> > +        -> [String]
> > +        -> IO ()
>
> I'm not sure about two parts of these patches:
>
> the "-----" lines seem unecessary to me and are the sort of thing that'll
> be
> added once in these clean-up patches but then never by anyone adding new
> functions. I would leave them out, personally. What purpose do they serve?
>

To demarcate 80 characters, and separate the functions until it's more
obvious. They can absolutely be left out, I was just having a hard time
navigating some of the absolutely huge functions that are in darcs. It's a
convention used by the Snap project. They're not important, though, and if
they're distracting, that's no good.


> Secondly, I think the wrapping of the type-signature is only necessary if
> the
> length is too great, or if you wish to add end-of-line comments to
> individual
> parameter types. Otherwise we're increasing the number of lines for no real
> benefit (that I can see).
>

The only reason I put them there is precisely the reason you give: to make
it easier to put Haddock comments on the parameters. If you check out
Tibbel's guide, under Comments, Top Level Definitions, that's my goal. Once
I have the format laid out, I'd like to provide comments to the parameters.
I feel like I should know the code better first, though.



> The reason I'm mentioning these is that I've been slightly pushing recently
> for
> Darcs to adopt Tibbe's style guide[1], which I find to be a nice "go to"
> for
> style questions - it's not really officially agreed on though. But it'd
> make
> sense if our clean up patches are following the same guidelines!
>

That's the style guide I'm using, so as long as you're accepting my
patches, we'll be there in no time!

Attachments

• unnamed

On 12 March 2012 14:06, Will Langstroth <will@langstroth.com> wrote:
> To demarcate 80 characters, and separate the functions until it's more
obvious. They can absolutely be left out, I was just having a hard time
navigating some of the absolutely huge functions that are in darcs. It's a
convention used by the Snap project. They're not important, though, and if
they're distracting, that's no good.

I'm not sure I'm convinced that they are useful. I do find that they are
distracting - if you're having a hard time visually picking out function
boundaries you could potentially use some sort of editor-highlighting.

> The only reason I put them there is precisely the reason you give: to
make it easier to put Haddock comments on the parameters. If you check out
Tibbel's guide, under Comments, Top Level Definitions, that's my goal. Once
I have the format laid out, I'd like to provide comments to the parameters.
I feel like I should know the code better first, though.

Ah ok. I suppose I would only expect them split like that if you actually do
comment each parameter, in the case when it isn't clear from the typename or
top-level haddock what their purpose is. I think commenting each and every
parameter type would probably be over-the-top.

> That's the style guide I'm using, so as long as you're accepting my
patches, we'll be there in no time!

Luckily, you knew which guide I meant, even with me forgetting the link :-)
Yeah, I hope so!

Cheers,
Owen.

Attachments

• unnamed

Owen Stephens <darcs@owenstephens.co.uk> added the comment:
> I'm not sure about two parts of these patches:
>
> the "-----" lines seem unecessary to me and are the sort of thing that'll be
> added once in these clean-up patches but then never by anyone adding new
> functions. I would leave them out, personally. What purpose do they serve?

FWIW, I noticed that Darcs.Annotate uses a similar convention.  I
hadn't seen it in Haskell code before, but found it surprisingly
useful while reading the code.

It looks like Darcs.Patch.Apply and Darcs.Patch.Merge use the convention also.

--
Michael

On 12 March 2012 14:34, Michael Hendricks <michael@ndrix.org> wrote:

> FWIW, I noticed that Darcs.Annotate uses a similar convention.  I
> hadn't seen it in Haskell code before, but found it surprisingly
> useful while reading the code.
>
> It looks like Darcs.Patch.Apply and Darcs.Patch.Merge use the convention
> also.
>

Indeed, looking at the history for those files, it appears that Will was the
originator (culprit? ;-))

I agree that sometimes it can be useful to separate the function bodies
visually, but I think that since not everyone needs/wants it, it doesn't
seem
right to include it "by default".

The only reason I really commented is that I was tidying up
Darcs/Commands/Add
yesterday night, and found myself removing them all, as they were irritating
(interpret that as you wish!).

I won't make too much fuss about it - the benefit of having Will working on
making a dent in the untidiness of Darcs' code far outweighs any personal
complaints I have about function "delimiters".

Cheers,
Owen.

Attachments

• unnamed

>
> Indeed, looking at the history for those files, it appears that Will was
> the
> originator (culprit? ;-))
>

It's true, that would be me. I'll go through and get rid of them, because I
think they're distracting people, and that's the opposite of readability.


> I won't make too much fuss about it - the benefit of having Will working on
> making a dent in the untidiness of Darcs' code far outweighs any personal
> complaints I have about function "delimiters".
>

One important reason for tidying up code is so that the people who are
working on it feel like they're working on something well made and
organized. It's a subtle thing, but I've seen it do wonders for
productivity and interest. If we weren't picky about the small details,
chances are, we wouldn't make for very good programmers! In that light,
aesthetics are important.

It's also really easy to take them out, so I'll do that.

Attachments

• unnamed

I share Owen's concern that the horizontal lines don't really help and
look sort of arbitrarily placed; otherwise the patches looks good.