www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Review of std.net.isemail part 2

reply Jacob Carlborg <doob me.com> writes:
I've made a few minor changes:

* Renamed EmailStatusCode.Off -> None and On -> Any
* Added and clarified the documentation for EmailStatusCode.Any and None
* Updated the documentation

Github: https://github.com/jacob-carlborg/phobos/tree/isemail
Docs: http://dl.dropbox.com/u/18386187/isemail.html

-- 
/Jacob Carlborg
Mar 29 2011
next sibling parent reply Jesse Phillips <jessekphillips+D gmail.com> writes:
Jacob Carlborg Wrote:


 I've made a few minor changes:
 
 * Renamed EmailStatusCode.Off -> None and On -> Any
 * Added and clarified the documentation for EmailStatusCode.Any and None
 * Updated the documentation
 
 Github: https://github.com/jacob-carlborg/phobos/tree/isemail
 Docs: http://dl.dropbox.com/u/18386187/isemail.html
 
 -- 
 /Jacob Carlborg



I believe enum values are to be named lowercase first. EmailStatusCode.any
Mar 29 2011
parent reply Jacob Carlborg <doob me.com> writes:
On 3/30/11 1:30 AM, Jesse Phillips wrote:

 Jacob Carlborg Wrote:


 I've made a few minor changes:

 * Renamed EmailStatusCode.Off ->  None and On ->  Any
 * Added and clarified the documentation for EmailStatusCode.Any and None
 * Updated the documentation

 Github: https://github.com/jacob-carlborg/phobos/tree/isemail
 Docs: http://dl.dropbox.com/u/18386187/isemail.html

 --
 /Jacob Carlborg



 I believe enum values are to be named lowercase first. EmailStatusCode.any


I don't know what the style guide says about enum members but if that's 
the case I'll change the names to begin with lowercase.

-- 
/Jacob Carlborg
Mar 30 2011
parent reply Jonathan M Davis <jmdavisProg gmx.com> writes:
On 2011-03-30 01:27, Jacob Carlborg wrote:

 On 3/30/11 1:30 AM, Jesse Phillips wrote:

 Jacob Carlborg Wrote:

 I've made a few minor changes:
 
 * Renamed EmailStatusCode.Off ->  None and On ->  Any
 * Added and clarified the documentation for EmailStatusCode.Any and None
 * Updated the documentation
 
 Github: https://github.com/jacob-carlborg/phobos/tree/isemail
 Docs: http://dl.dropbox.com/u/18386187/isemail.html
 
 --
 /Jacob Carlborg


 
 I believe enum values are to be named lowercase first.
 EmailStatusCode.any


 
 I don't know what the style guide says about enum members but if that's
 the case I'll change the names to begin with lowercase.


All names are camelcased. All type names begin with an uppercase letter, and 
all variables begin with a lowercase letter (with the possible exception of 
private member variables beginning with _ - but what's private to a class or 
struct isn't as critical as the public API regardless). enum values fall in 
the same camp as variables.

We don't really have an actual document anywhere that lays out the style. 
Andrei has resisted the idea, though I don't think that he's entirely against 
it, so unfortunately, I don't think that there's currently anywhere that you 
can look it up. Discussions of the newsgroups have made the decisions on the 
naming conventions clear though, at least as long as you've paid attention and 
remember them, which obviously isn't going to happen for newer folks on the 
list, and unless you're regularly writing Phobos code and making sure that you 
follow the appropriate conventions, it's probably not all that hard to forget 
them if they're not what you'd do anyway.

In any case, enum values follow the same style as normal variables - 
camelcased, beginning with a lowercase letter.

- Jonathan M Davis
Mar 30 2011
next sibling parent reply Don <nospam nospam.com> writes:
Jonathan M Davis wrote:

 On 2011-03-30 01:27, Jacob Carlborg wrote:

 On 3/30/11 1:30 AM, Jesse Phillips wrote:

 Jacob Carlborg Wrote:

 I've made a few minor changes:

 * Renamed EmailStatusCode.Off ->  None and On ->  Any
 * Added and clarified the documentation for EmailStatusCode.Any and None
 * Updated the documentation

 Github: https://github.com/jacob-carlborg/phobos/tree/isemail
 Docs: http://dl.dropbox.com/u/18386187/isemail.html

 --
 /Jacob Carlborg


 I believe enum values are to be named lowercase first.
 EmailStatusCode.any


 I don't know what the style guide says about enum members but if that's
 the case I'll change the names to begin with lowercase.


 
 All names are camelcased. 


That's not true. ALLCAPS is relatively common in Phobos.
There is absolutely no way PI is going to become pi.


 All type names begin with an uppercase letter, and 
 all variables begin with a lowercase letter (with the possible exception of 
 private member variables beginning with _ - but what's private to a class or 
 struct isn't as critical as the public API regardless). 


That part is clear.


 enum values fall in the same camp as variables.


I never heard that before, and it doesn't seem to be true throughout 
Phobos. Grepping for all enum declarations (there isn't very many of 
them actually), I found some which were like that, some which start with 
uppercase, and some which are all caps.

I think you're assuming more concensus on style than has ever actually 
been discussed.
Mar 30 2011
next sibling parent reply "Lars T. Kyllingstad" <public kyllingen.NOSPAMnet> writes:
On Wed, 30 Mar 2011 13:23:02 +0200, Don wrote:


 Jonathan M Davis wrote:

 On 2011-03-30 01:27, Jacob Carlborg wrote:

 On 3/30/11 1:30 AM, Jesse Phillips wrote:

 Jacob Carlborg Wrote:

 I've made a few minor changes:

 * Renamed EmailStatusCode.Off ->  None and On ->  Any * Added and
 clarified the documentation for EmailStatusCode.Any and None *
 Updated the documentation

 Github: https://github.com/jacob-carlborg/phobos/tree/isemail Docs:
 http://dl.dropbox.com/u/18386187/isemail.html

 --
 /Jacob Carlborg


 I believe enum values are to be named lowercase first.
 EmailStatusCode.any


 I don't know what the style guide says about enum members but if
 that's the case I'll change the names to begin with lowercase.


 
 All names are camelcased.


 
 That's not true. ALLCAPS is relatively common in Phobos. There is
 absolutely no way PI is going to become pi.
 

 All type names begin with an uppercase letter, and all variables begin
 with a lowercase letter (with the possible exception of private member
 variables beginning with _ - but what's private to a class or struct
 isn't as critical as the public API regardless).


 
 That part is clear.
 
  > enum values fall in the same camp as variables.
 
 I never heard that before, and it doesn't seem to be true throughout
 Phobos. Grepping for all enum declarations (there isn't very many of
 them actually), I found some which were like that, some which start with
 uppercase, and some which are all caps.
 
 I think you're assuming more concensus on style than has ever actually
 been discussed.



I think Andrei introduced the camelCase enum convention with his Phobos 
overhaul back in 2.029.  All new modules, and most modules which have 
seen major changes since then, follow it -- at least in the public API.  
Examples include std.algorithm, std.datetime, std.file, std.getopt, 
std.range and std.stdio.

I wouldn't mind if PI became pi -- I'd never dream of naming a variable 
pi anyway, unless it's actually supposed to represent π.  Renaming E to 
e, on the other hand, that's a lot worse.

-Lars
Mar 30 2011
parent reply Don <nospam nospam.com> writes:
Lars T. Kyllingstad wrote:

 On Wed, 30 Mar 2011 13:23:02 +0200, Don wrote:
 

 Jonathan M Davis wrote:

 On 2011-03-30 01:27, Jacob Carlborg wrote:

 On 3/30/11 1:30 AM, Jesse Phillips wrote:

 Jacob Carlborg Wrote:

 I've made a few minor changes:

 * Renamed EmailStatusCode.Off ->  None and On ->  Any * Added and
 clarified the documentation for EmailStatusCode.Any and None *
 Updated the documentation

 Github: https://github.com/jacob-carlborg/phobos/tree/isemail Docs:
 http://dl.dropbox.com/u/18386187/isemail.html

 --
 /Jacob Carlborg


 I believe enum values are to be named lowercase first.
 EmailStatusCode.any


 I don't know what the style guide says about enum members but if
 that's the case I'll change the names to begin with lowercase.


 All names are camelcased.


 That's not true. ALLCAPS is relatively common in Phobos. There is
 absolutely no way PI is going to become pi.


 All type names begin with an uppercase letter, and all variables begin
 with a lowercase letter (with the possible exception of private member
 variables beginning with _ - but what's private to a class or struct
 isn't as critical as the public API regardless).


 That part is clear.

  > enum values fall in the same camp as variables.

 I never heard that before, and it doesn't seem to be true throughout
 Phobos. Grepping for all enum declarations (there isn't very many of
 them actually), I found some which were like that, some which start with
 uppercase, and some which are all caps.

 I think you're assuming more concensus on style than has ever actually
 been discussed.


 
 
 I think Andrei introduced the camelCase enum convention with his Phobos 
 overhaul back in 2.029.  All new modules, and most modules which have 
 seen major changes since then, follow it -- at least in the public API.  
 Examples include std.algorithm, std.datetime, std.file, std.getopt, 
 std.range and std.stdio.
 
 I wouldn't mind if PI became pi -- I'd never dream of naming a variable 
 pi anyway, unless it's actually supposed to represent π.  Renaming E to 
 e, on the other hand, that's a lot worse.
 
 -Lars


Hardly. The only examples I could find were
algorithm: SwapStrategy, SortOutput
range: traverseOptions, SearchPolicy
There are many more which use other conventions, in other modules.

Note that manifest constants (eg, enum int XXX = value;) are completely 
different to enumerated types (eg, enum XXX { AAA, BBB, CCC } )
They're using a keyword in common, but they're quite different concepts.
Mar 30 2011
parent reply "Lars T. Kyllingstad" <public kyllingen.NOSPAMnet> writes:
On Wed, 30 Mar 2011 15:04:49 +0200, Don wrote:


 Lars T. Kyllingstad wrote:

 On Wed, 30 Mar 2011 13:23:02 +0200, Don wrote:
 

 Jonathan M Davis wrote:

 On 2011-03-30 01:27, Jacob Carlborg wrote:

 On 3/30/11 1:30 AM, Jesse Phillips wrote:

 Jacob Carlborg Wrote:

 I've made a few minor changes:

 * Renamed EmailStatusCode.Off ->  None and On ->  Any * Added and
 clarified the documentation for EmailStatusCode.Any and None *
 Updated the documentation

 Github: https://github.com/jacob-carlborg/phobos/tree/isemail
 Docs: http://dl.dropbox.com/u/18386187/isemail.html

 --
 /Jacob Carlborg


 I believe enum values are to be named lowercase first.
 EmailStatusCode.any


 I don't know what the style guide says about enum members but if
 that's the case I'll change the names to begin with lowercase.


 All names are camelcased.


 That's not true. ALLCAPS is relatively common in Phobos. There is
 absolutely no way PI is going to become pi.


 All type names begin with an uppercase letter, and all variables
 begin with a lowercase letter (with the possible exception of private
 member variables beginning with _ - but what's private to a class or
 struct isn't as critical as the public API regardless).


 That part is clear.

  > enum values fall in the same camp as variables.

 I never heard that before, and it doesn't seem to be true throughout
 Phobos. Grepping for all enum declarations (there isn't very many of
 them actually), I found some which were like that, some which start
 with uppercase, and some which are all caps.

 I think you're assuming more concensus on style than has ever actually
 been discussed.


 
 
 I think Andrei introduced the camelCase enum convention with his Phobos
 overhaul back in 2.029.  All new modules, and most modules which have
 seen major changes since then, follow it -- at least in the public API.
 Examples include std.algorithm, std.datetime, std.file, std.getopt,
 std.range and std.stdio.
 
 I wouldn't mind if PI became pi -- I'd never dream of naming a variable
 pi anyway, unless it's actually supposed to represent π.  Renaming E to
 e, on the other hand, that's a lot worse.
 
 -Lars


 
 Hardly. The only examples I could find were algorithm: SwapStrategy,
 SortOutput
 range: traverseOptions, SearchPolicy
 There are many more which use other conventions, in other modules.


I don't intend to start a big debate about this, but I don't think you 
looked very hard.  All of the modules I mentioned follow the camelCase 
convention, and as far as I can tell, none have public enums that follow 
other conventions.

std.algorithm: OpenRight, EditOp, SwapStrategy, SortOutput

std.datetime: Month, DayOfWeek, AllowDayOverflow, Direction, PopFirst, 
AutoStart

std.file: SpanMode

std.getopt: config (actually not conventional, should be Config, but its 
members are still camelCased)

std.range: StoppingPolicy, TransverseOptions, SearchPolicy

std.stdio: KeepTerminator



 Note that manifest constants (eg, enum int XXX = value;) are completely
 different to enumerated types (eg, enum XXX { AAA, BBB, CCC } ) They're
 using a keyword in common, but they're quite different concepts.


I think a sensible rule is that types (and templates that evaluate to 
types) start with a capital letter, while values (and functions/templates 
that evaluate to values) are camelCased.

-Lars
Mar 30 2011
parent reply Don <nospam nospam.com> writes:
Lars T. Kyllingstad wrote:

 On Wed, 30 Mar 2011 15:04:49 +0200, Don wrote:
 

 Lars T. Kyllingstad wrote:

 On Wed, 30 Mar 2011 13:23:02 +0200, Don wrote:


 Jonathan M Davis wrote:

 On 2011-03-30 01:27, Jacob Carlborg wrote:

 On 3/30/11 1:30 AM, Jesse Phillips wrote:

 Jacob Carlborg Wrote:

 I've made a few minor changes:

 * Renamed EmailStatusCode.Off ->  None and On ->  Any * Added and
 clarified the documentation for EmailStatusCode.Any and None *
 Updated the documentation

 Github: https://github.com/jacob-carlborg/phobos/tree/isemail
 Docs: http://dl.dropbox.com/u/18386187/isemail.html

 --
 /Jacob Carlborg


 I believe enum values are to be named lowercase first.
 EmailStatusCode.any


 I don't know what the style guide says about enum members but if
 that's the case I'll change the names to begin with lowercase.


 All names are camelcased.


 That's not true. ALLCAPS is relatively common in Phobos. There is
 absolutely no way PI is going to become pi.


 All type names begin with an uppercase letter, and all variables
 begin with a lowercase letter (with the possible exception of private
 member variables beginning with _ - but what's private to a class or
 struct isn't as critical as the public API regardless).


 That part is clear.

  > enum values fall in the same camp as variables.

 I never heard that before, and it doesn't seem to be true throughout
 Phobos. Grepping for all enum declarations (there isn't very many of
 them actually), I found some which were like that, some which start
 with uppercase, and some which are all caps.

 I think you're assuming more concensus on style than has ever actually
 been discussed.


 I think Andrei introduced the camelCase enum convention with his Phobos
 overhaul back in 2.029.  All new modules, and most modules which have
 seen major changes since then, follow it -- at least in the public API.
 Examples include std.algorithm, std.datetime, std.file, std.getopt,
 std.range and std.stdio.

 I wouldn't mind if PI became pi -- I'd never dream of naming a variable
 pi anyway, unless it's actually supposed to represent π.  Renaming E to
 e, on the other hand, that's a lot worse.

 -Lars


 Hardly. The only examples I could find were algorithm: SwapStrategy,
 SortOutput
 range: traverseOptions, SearchPolicy
 There are many more which use other conventions, in other modules.


 
 I don't intend to start a big debate about this, but I don't think you 
 looked very hard.  All of the modules I mentioned follow the camelCase 
 convention, and as far as I can tell, none have public enums that follow 
 other conventions.
 
 std.algorithm: OpenRight, EditOp, SwapStrategy, SortOutput
 
 std.datetime: Month, DayOfWeek, AllowDayOverflow, Direction, PopFirst, 
 AutoStart
 std.file: SpanMode
 
 std.getopt: config (actually not conventional, should be Config, but its 
 members are still camelCased)
 
 std.range: StoppingPolicy, TransverseOptions, SearchPolicy
 
 std.stdio: KeepTerminator



 
 

 Note that manifest constants (eg, enum int XXX = value;) are completely
 different to enumerated types (eg, enum XXX { AAA, BBB, CCC } ) They're
 using a keyword in common, but they're quite different concepts.


 
 I think a sensible rule is that types (and templates that evaluate to 
 types) start with a capital letter, while values (and functions/templates 
 that evaluate to values) are camelCased.
 
 -Lars


The point is this: we do NOT have a style guide.
We have consensus on a few things. Types start with a capital letter. 
Functions are camelCased. Many other things haven't actually been 
discussed and agreed to.

Note that simplistic rules are doomed to failure. For example, template 
aliases can be either values or types.

Also, any attempt to use precedent is a disaster since Phobos began as a 
complete mishmash of styles. In some cases people erroneously believed 
there was a convention, and attempted to adhere to it, even though no 
such convention existed.

We desperately need a style guide containing all the things which have 
actually been agreed to (and equally importantly, nothing which hasn't).
The simplest thing to do would be to fix up the existing one on the 
website which nobody follows.
Mar 30 2011
next sibling parent reply Daniel Gibson <metalcaedes gmail.com> writes:
Am 30.03.2011 20:15, schrieb Don:

 Lars T. Kyllingstad wrote:

 On Wed, 30 Mar 2011 15:04:49 +0200, Don wrote:


 Lars T. Kyllingstad wrote:

 On Wed, 30 Mar 2011 13:23:02 +0200, Don wrote:


 Jonathan M Davis wrote:

 On 2011-03-30 01:27, Jacob Carlborg wrote:

 On 3/30/11 1:30 AM, Jesse Phillips wrote:

 Jacob Carlborg Wrote:

 I've made a few minor changes:

 * Renamed EmailStatusCode.Off -> None and On -> Any * Added and
 clarified the documentation for EmailStatusCode.Any and None *
 Updated the documentation

 Github: https://github.com/jacob-carlborg/phobos/tree/isemail
 Docs: http://dl.dropbox.com/u/18386187/isemail.html

 --
 /Jacob Carlborg


 I believe enum values are to be named lowercase first.
 EmailStatusCode.any


 I don't know what the style guide says about enum members but if
 that's the case I'll change the names to begin with lowercase.


 All names are camelcased.


 That's not true. ALLCAPS is relatively common in Phobos. There is
 absolutely no way PI is going to become pi.


 All type names begin with an uppercase letter, and all variables
 begin with a lowercase letter (with the possible exception of private
 member variables beginning with _ - but what's private to a class or
 struct isn't as critical as the public API regardless).


 That part is clear.


 enum values fall in the same camp as variables.


 I never heard that before, and it doesn't seem to be true throughout
 Phobos. Grepping for all enum declarations (there isn't very many of
 them actually), I found some which were like that, some which start
 with uppercase, and some which are all caps.

 I think you're assuming more concensus on style than has ever actually
 been discussed.


 I think Andrei introduced the camelCase enum convention with his Phobos
 overhaul back in 2.029. All new modules, and most modules which have
 seen major changes since then, follow it -- at least in the public API.
 Examples include std.algorithm, std.datetime, std.file, std.getopt,
 std.range and std.stdio.

 I wouldn't mind if PI became pi -- I'd never dream of naming a variable
 pi anyway, unless it's actually supposed to represent π. Renaming E to
 e, on the other hand, that's a lot worse.

 -Lars


 Hardly. The only examples I could find were algorithm: SwapStrategy,
 SortOutput
 range: traverseOptions, SearchPolicy
 There are many more which use other conventions, in other modules.


 I don't intend to start a big debate about this, but I don't think you
 looked very hard. All of the modules I mentioned follow the camelCase
 convention, and as far as I can tell, none have public enums that
 follow other conventions.

 std.algorithm: OpenRight, EditOp, SwapStrategy, SortOutput

 std.datetime: Month, DayOfWeek, AllowDayOverflow, Direction, PopFirst,
 AutoStart
 std.file: SpanMode

 std.getopt: config (actually not conventional, should be Config, but
 its members are still camelCased)

 std.range: StoppingPolicy, TransverseOptions, SearchPolicy

 std.stdio: KeepTerminator



 Note that manifest constants (eg, enum int XXX = value;) are completely
 different to enumerated types (eg, enum XXX { AAA, BBB, CCC } ) They're
 using a keyword in common, but they're quite different concepts.


 I think a sensible rule is that types (and templates that evaluate to
 types) start with a capital letter, while values (and
 functions/templates that evaluate to values) are camelCased.

 -Lars


 The point is this: we do NOT have a style guide.
 We have consensus on a few things. Types start with a capital letter.
 Functions are camelCased. Many other things haven't actually been
 discussed and agreed to.

 Note that simplistic rules are doomed to failure. For example, template
 aliases can be either values or types.

 Also, any attempt to use precedent is a disaster since Phobos began as a
 complete mishmash of styles. In some cases people erroneously believed
 there was a convention, and attempted to adhere to it, even though no
 such convention existed.

 We desperately need a style guide containing all the things which have
 actually been agreed to (and equally importantly, nothing which hasn't).
 The simplest thing to do would be to fix up the existing one on the
 website which nobody follows.



What about http://digitalmars.com/d/2.0/dstyle.html ?
Mar 30 2011
next sibling parent reply "Simen kjaeraas" <simen.kjaras gmail.com> writes:
On Wed, 30 Mar 2011 20:19:36 +0200, Daniel Gibson <metalcaedes gmail.com>  
wrote:


 We desperately need a style guide containing all the things which have
 actually been agreed to (and equally importantly, nothing which hasn't).
 The simplest thing to do would be to fix up the existing one on the
 website which nobody follows.



 What about http://digitalmars.com/d/2.0/dstyle.html ?


That document has little to do with actual D code written and included in
Phobos or elsewhere. We really need a new guide.

-- 
Simen
Mar 30 2011
parent KennyTM~ <kennytm gmail.com> writes:
On Mar 31, 11 02:22, Simen kjaeraas wrote:

 On Wed, 30 Mar 2011 20:19:36 +0200, Daniel Gibson
 <metalcaedes gmail.com> wrote:


 We desperately need a style guide containing all the things which have
 actually been agreed to (and equally importantly, nothing which hasn't).
 The simplest thing to do would be to fix up the existing one on the
 website which nobody follows.



 What about http://digitalmars.com/d/2.0/dstyle.html ?


 That document has little to do with actual D code written and included in
 Phobos or elsewhere. We really need a new guide.


Is this still the case? The issue has been brought up several times in 
this NG and I thought it should have been fixed.

Except 'Two blank lines separating function bodies' the new Phobos 
modules (just looking at container.d) conform to this style guide quite 
well.
Mar 30 2011
prev sibling parent Jonathan M Davis <jmdavisProg gmx.com> writes:
On 2011-03-30 11:19, Daniel Gibson wrote:

 Am 30.03.2011 20:15, schrieb Don:

 Lars T. Kyllingstad wrote:

 On Wed, 30 Mar 2011 15:04:49 +0200, Don wrote:

 Lars T. Kyllingstad wrote:

 On Wed, 30 Mar 2011 13:23:02 +0200, Don wrote:

 Jonathan M Davis wrote:

 On 2011-03-30 01:27, Jacob Carlborg wrote:

 On 3/30/11 1:30 AM, Jesse Phillips wrote:

 Jacob Carlborg Wrote:

 I've made a few minor changes:
=20
 * Renamed EmailStatusCode.Off -> None and On -> Any * Added and
 clarified the documentation for EmailStatusCode.Any and None *
 Updated the documentation
=20
 Github: https://github.com/jacob-carlborg/phobos/tree/isemail
 Docs: http://dl.dropbox.com/u/18386187/isemail.html
=20
 --
 /Jacob Carlborg


=20
 I believe enum values are to be named lowercase first.
 EmailStatusCode.any


=20
 I don't know what the style guide says about enum members but if
 that's the case I'll change the names to begin with lowercase.


=20
 All names are camelcased.


=20
 That's not true. ALLCAPS is relatively common in Phobos. There is
 absolutely no way PI is going to become pi.
=20

 All type names begin with an uppercase letter, and all variables
 begin with a lowercase letter (with the possible exception of
 private member variables beginning with _ - but what's private to a
 class or struct isn't as critical as the public API regardless).


=20
 That part is clear.
=20

 enum values fall in the same camp as variables.


=20
 I never heard that before, and it doesn't seem to be true throughout
 Phobos. Grepping for all enum declarations (there isn't very many of
 them actually), I found some which were like that, some which start
 with uppercase, and some which are all caps.
=20
 I think you're assuming more concensus on style than has ever
 actually been discussed.


=20
 I think Andrei introduced the camelCase enum convention with his
 Phobos overhaul back in 2.029. All new modules, and most modules
 which have seen major changes since then, follow it -- at least in
 the public API. Examples include std.algorithm, std.datetime,
 std.file, std.getopt, std.range and std.stdio.
=20
 I wouldn't mind if PI became pi -- I'd never dream of naming a
 variable pi anyway, unless it's actually supposed to represent =CF=










=80.

 Renaming E to e, on the other hand, that's a lot worse.
=20
 -Lars


=20
 Hardly. The only examples I could find were algorithm: SwapStrategy,
 SortOutput
 range: traverseOptions, SearchPolicy
 There are many more which use other conventions, in other modules.


=20
 I don't intend to start a big debate about this, but I don't think you
 looked very hard. All of the modules I mentioned follow the camelCase
 convention, and as far as I can tell, none have public enums that
 follow other conventions.
=20
 std.algorithm: OpenRight, EditOp, SwapStrategy, SortOutput
=20
 std.datetime: Month, DayOfWeek, AllowDayOverflow, Direction, PopFirst,
 AutoStart
 std.file: SpanMode
=20
 std.getopt: config (actually not conventional, should be Config, but
 its members are still camelCased)
=20
 std.range: StoppingPolicy, TransverseOptions, SearchPolicy
=20
 std.stdio: KeepTerminator
=20

 Note that manifest constants (eg, enum int XXX =3D value;) are comple=








tely

 different to enumerated types (eg, enum XXX { AAA, BBB, CCC } ) They'=








re

 using a keyword in common, but they're quite different concepts.


=20
 I think a sensible rule is that types (and templates that evaluate to
 types) start with a capital letter, while values (and
 functions/templates that evaluate to values) are camelCased.
=20
 -Lars


=20
 The point is this: we do NOT have a style guide.
 We have consensus on a few things. Types start with a capital letter.
 Functions are camelCased. Many other things haven't actually been
 discussed and agreed to.
=20
 Note that simplistic rules are doomed to failure. For example, template
 aliases can be either values or types.
=20
 Also, any attempt to use precedent is a disaster since Phobos began as a
 complete mishmash of styles. In some cases people erroneously believed
 there was a convention, and attempted to adhere to it, even though no
 such convention existed.
=20
 We desperately need a style guide containing all the things which have
 actually been agreed to (and equally importantly, nothing which hasn't).
 The simplest thing to do would be to fix up the existing one on the
 website which nobody follows.


=20
 What about http://digitalmars.com/d/2.0/dstyle.html ?


That would be the "existing one on the website which nobody follows." It's=
=20
actually not all that far off from what Phobos actually does, but it's not=
=20
really actually followed, and it wasn't agreed on by the Phobos devs. Also,=
=20
what we're looking for is a _Phobos_ style guide, not a _D_ style guide. To=
=20
some extent at least, it would be nice if a lot of Phobos' conventions=20
(particularly naming conventions) were used by the community at large, but=
=20
we're not looking to enforce that at all. What we're looking for is=20
consistency within Phobos. And we need an actual style guide for that, sinc=
e=20
even if all of the current Phobos devs agree on a set of conventions and=20
follow them religiously, no one new to the team or who is writing code with=
=20
the hope of getting it into Phobos is necessarily going to be aware of thos=
e=20
conventions.

=2D Jonathan M Davis
Mar 30 2011
prev sibling next sibling parent reply Jacob Carlborg <doob me.com> writes:
On 2011-03-30 20:15, Don wrote:

 Lars T. Kyllingstad wrote:

 On Wed, 30 Mar 2011 15:04:49 +0200, Don wrote:


 Lars T. Kyllingstad wrote:

 On Wed, 30 Mar 2011 13:23:02 +0200, Don wrote:


 Jonathan M Davis wrote:

 On 2011-03-30 01:27, Jacob Carlborg wrote:

 On 3/30/11 1:30 AM, Jesse Phillips wrote:

 Jacob Carlborg Wrote:

 I've made a few minor changes:

 * Renamed EmailStatusCode.Off -> None and On -> Any * Added and
 clarified the documentation for EmailStatusCode.Any and None *
 Updated the documentation

 Github: https://github.com/jacob-carlborg/phobos/tree/isemail
 Docs: http://dl.dropbox.com/u/18386187/isemail.html

 --
 /Jacob Carlborg


 I believe enum values are to be named lowercase first.
 EmailStatusCode.any


 I don't know what the style guide says about enum members but if
 that's the case I'll change the names to begin with lowercase.


 All names are camelcased.


 That's not true. ALLCAPS is relatively common in Phobos. There is
 absolutely no way PI is going to become pi.


 All type names begin with an uppercase letter, and all variables
 begin with a lowercase letter (with the possible exception of private
 member variables beginning with _ - but what's private to a class or
 struct isn't as critical as the public API regardless).


 That part is clear.


 enum values fall in the same camp as variables.


 I never heard that before, and it doesn't seem to be true throughout
 Phobos. Grepping for all enum declarations (there isn't very many of
 them actually), I found some which were like that, some which start
 with uppercase, and some which are all caps.

 I think you're assuming more concensus on style than has ever actually
 been discussed.


 I think Andrei introduced the camelCase enum convention with his Phobos
 overhaul back in 2.029. All new modules, and most modules which have
 seen major changes since then, follow it -- at least in the public API.
 Examples include std.algorithm, std.datetime, std.file, std.getopt,
 std.range and std.stdio.

 I wouldn't mind if PI became pi -- I'd never dream of naming a variable
 pi anyway, unless it's actually supposed to represent π. Renaming E to
 e, on the other hand, that's a lot worse.

 -Lars


 Hardly. The only examples I could find were algorithm: SwapStrategy,
 SortOutput
 range: traverseOptions, SearchPolicy
 There are many more which use other conventions, in other modules.


 I don't intend to start a big debate about this, but I don't think you
 looked very hard. All of the modules I mentioned follow the camelCase
 convention, and as far as I can tell, none have public enums that
 follow other conventions.

 std.algorithm: OpenRight, EditOp, SwapStrategy, SortOutput

 std.datetime: Month, DayOfWeek, AllowDayOverflow, Direction, PopFirst,
 AutoStart
 std.file: SpanMode

 std.getopt: config (actually not conventional, should be Config, but
 its members are still camelCased)

 std.range: StoppingPolicy, TransverseOptions, SearchPolicy

 std.stdio: KeepTerminator



 Note that manifest constants (eg, enum int XXX = value;) are completely
 different to enumerated types (eg, enum XXX { AAA, BBB, CCC } ) They're
 using a keyword in common, but they're quite different concepts.


 I think a sensible rule is that types (and templates that evaluate to
 types) start with a capital letter, while values (and
 functions/templates that evaluate to values) are camelCased.

 -Lars


 The point is this: we do NOT have a style guide.
 We have consensus on a few things. Types start with a capital letter.
 Functions are camelCased. Many other things haven't actually been
 discussed and agreed to.

 Note that simplistic rules are doomed to failure. For example, template
 aliases can be either values or types.

 Also, any attempt to use precedent is a disaster since Phobos began as a
 complete mishmash of styles. In some cases people erroneously believed
 there was a convention, and attempted to adhere to it, even though no
 such convention existed.

 We desperately need a style guide containing all the things which have
 actually been agreed to (and equally importantly, nothing which hasn't).
 The simplest thing to do would be to fix up the existing one on the
 website which nobody follows.


So, should I change the enum members to start with lowercase or leave it 
like it is?

-- 
/Jacob Carlborg
Mar 30 2011
next sibling parent Jonathan M Davis <jmdavisProg gmx.com> writes:
On 2011-03-30 11:47, Jacob Carlborg wrote:

 On 2011-03-30 20:15, Don wrote:

 Lars T. Kyllingstad wrote:

 On Wed, 30 Mar 2011 15:04:49 +0200, Don wrote:

 Lars T. Kyllingstad wrote:

 On Wed, 30 Mar 2011 13:23:02 +0200, Don wrote:

 Jonathan M Davis wrote:

 On 2011-03-30 01:27, Jacob Carlborg wrote:

 On 3/30/11 1:30 AM, Jesse Phillips wrote:

 Jacob Carlborg Wrote:

 I've made a few minor changes:
=20
 * Renamed EmailStatusCode.Off -> None and On -> Any * Added and
 clarified the documentation for EmailStatusCode.Any and None *
 Updated the documentation
=20
 Github: https://github.com/jacob-carlborg/phobos/tree/isemail
 Docs: http://dl.dropbox.com/u/18386187/isemail.html
=20
 --
 /Jacob Carlborg


=20
 I believe enum values are to be named lowercase first.
 EmailStatusCode.any


=20
 I don't know what the style guide says about enum members but if
 that's the case I'll change the names to begin with lowercase.


=20
 All names are camelcased.


=20
 That's not true. ALLCAPS is relatively common in Phobos. There is
 absolutely no way PI is going to become pi.
=20

 All type names begin with an uppercase letter, and all variables
 begin with a lowercase letter (with the possible exception of
 private member variables beginning with _ - but what's private to a
 class or struct isn't as critical as the public API regardless).


=20
 That part is clear.
=20

 enum values fall in the same camp as variables.


=20
 I never heard that before, and it doesn't seem to be true throughout
 Phobos. Grepping for all enum declarations (there isn't very many of
 them actually), I found some which were like that, some which start
 with uppercase, and some which are all caps.
=20
 I think you're assuming more concensus on style than has ever
 actually been discussed.


=20
 I think Andrei introduced the camelCase enum convention with his
 Phobos overhaul back in 2.029. All new modules, and most modules
 which have seen major changes since then, follow it -- at least in
 the public API. Examples include std.algorithm, std.datetime,
 std.file, std.getopt, std.range and std.stdio.
=20
 I wouldn't mind if PI became pi -- I'd never dream of naming a
 variable pi anyway, unless it's actually supposed to represent =CF=










=80.

 Renaming E to e, on the other hand, that's a lot worse.
=20
 -Lars


=20
 Hardly. The only examples I could find were algorithm: SwapStrategy,
 SortOutput
 range: traverseOptions, SearchPolicy
 There are many more which use other conventions, in other modules.


=20
 I don't intend to start a big debate about this, but I don't think you
 looked very hard. All of the modules I mentioned follow the camelCase
 convention, and as far as I can tell, none have public enums that
 follow other conventions.
=20
 std.algorithm: OpenRight, EditOp, SwapStrategy, SortOutput
=20
 std.datetime: Month, DayOfWeek, AllowDayOverflow, Direction, PopFirst,
 AutoStart
 std.file: SpanMode
=20
 std.getopt: config (actually not conventional, should be Config, but
 its members are still camelCased)
=20
 std.range: StoppingPolicy, TransverseOptions, SearchPolicy
=20
 std.stdio: KeepTerminator
=20

 Note that manifest constants (eg, enum int XXX =3D value;) are comple=








tely

 different to enumerated types (eg, enum XXX { AAA, BBB, CCC } ) They'=








re

 using a keyword in common, but they're quite different concepts.


=20
 I think a sensible rule is that types (and templates that evaluate to
 types) start with a capital letter, while values (and
 functions/templates that evaluate to values) are camelCased.
=20
 -Lars


=20
 The point is this: we do NOT have a style guide.
 We have consensus on a few things. Types start with a capital letter.
 Functions are camelCased. Many other things haven't actually been
 discussed and agreed to.
=20
 Note that simplistic rules are doomed to failure. For example, template
 aliases can be either values or types.
=20
 Also, any attempt to use precedent is a disaster since Phobos began as a
 complete mishmash of styles. In some cases people erroneously believed
 there was a convention, and attempted to adhere to it, even though no
 such convention existed.
=20
 We desperately need a style guide containing all the things which have
 actually been agreed to (and equally importantly, nothing which hasn't).
 The simplest thing to do would be to fix up the existing one on the
 website which nobody follows.


=20
 So, should I change the enum members to start with lowercase or leave it
 like it is?


I would say to change it to start with lowercase. That's what all of the ne=
wer=20
Phobos code does.

=2D Jonathan M Davis
Mar 30 2011
prev sibling parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 3/30/11 1:47 PM, Jacob Carlborg wrote:

 So, should I change the enum members to start with lowercase or leave it
 like it is?


Change please.

Thanks,

Andrei
Mar 30 2011
parent Jacob Carlborg <doob me.com> writes:
On 3/30/11 9:42 PM, Andrei Alexandrescu wrote:

 On 3/30/11 1:47 PM, Jacob Carlborg wrote:

 So, should I change the enum members to start with lowercase or leave it
 like it is?


 Change please.

 Thanks,

 Andrei


Ok, will do.

-- 
/Jacob Carlborg
Mar 30 2011
prev sibling parent reply Jonathan M Davis <jmdavisProg gmx.com> writes:
On 2011-03-30 11:15, Don wrote:

 Lars T. Kyllingstad wrote:

 On Wed, 30 Mar 2011 15:04:49 +0200, Don wrote:

 Lars T. Kyllingstad wrote:

 On Wed, 30 Mar 2011 13:23:02 +0200, Don wrote:

 Jonathan M Davis wrote:

 On 2011-03-30 01:27, Jacob Carlborg wrote:

 On 3/30/11 1:30 AM, Jesse Phillips wrote:

 Jacob Carlborg Wrote:

 I've made a few minor changes:
=20
 * Renamed EmailStatusCode.Off ->  None and On ->  Any * Added and
 clarified the documentation for EmailStatusCode.Any and None *
 Updated the documentation
=20
 Github: https://github.com/jacob-carlborg/phobos/tree/isemail
 Docs: http://dl.dropbox.com/u/18386187/isemail.html
=20
 --
 /Jacob Carlborg


=20
 I believe enum values are to be named lowercase first.
 EmailStatusCode.any


=20
 I don't know what the style guide says about enum members but if
 that's the case I'll change the names to begin with lowercase.


=20
 All names are camelcased.


=20
 That's not true. ALLCAPS is relatively common in Phobos. There is
 absolutely no way PI is going to become pi.
=20

 All type names begin with an uppercase letter, and all variables
 begin with a lowercase letter (with the possible exception of priva=












te

 member variables beginning with _ - but what's private to a class or
 struct isn't as critical as the public API regardless).


=20
 That part is clear.
=20
  > enum values fall in the same camp as variables.
=20
 I never heard that before, and it doesn't seem to be true throughout
 Phobos. Grepping for all enum declarations (there isn't very many of
 them actually), I found some which were like that, some which start
 with uppercase, and some which are all caps.
=20
 I think you're assuming more concensus on style than has ever actual=










ly

 been discussed.


=20
 I think Andrei introduced the camelCase enum convention with his Phob=








os

 overhaul back in 2.029.  All new modules, and most modules which have
 seen major changes since then, follow it -- at least in the public AP=








I.

 Examples include std.algorithm, std.datetime, std.file, std.getopt,
 std.range and std.stdio.
=20
 I wouldn't mind if PI became pi -- I'd never dream of naming a variab=








le

 pi anyway, unless it's actually supposed to represent =CF=80.  Renami=








ng E to

 e, on the other hand, that's a lot worse.
=20
 -Lars


=20
 Hardly. The only examples I could find were algorithm: SwapStrategy,
 SortOutput
 range: traverseOptions, SearchPolicy
 There are many more which use other conventions, in other modules.


=20
 I don't intend to start a big debate about this, but I don't think you
 looked very hard.  All of the modules I mentioned follow the camelCase
 convention, and as far as I can tell, none have public enums that follow
 other conventions.
=20
 std.algorithm: OpenRight, EditOp, SwapStrategy, SortOutput
=20
 std.datetime: Month, DayOfWeek, AllowDayOverflow, Direction, PopFirst,
 AutoStart
 std.file: SpanMode
=20
 std.getopt: config (actually not conventional, should be Config, but its
 members are still camelCased)
=20
 std.range: StoppingPolicy, TransverseOptions, SearchPolicy
=20
 std.stdio: KeepTerminator
=20

 Note that manifest constants (eg, enum int XXX =3D value;) are complet=






ely

 different to enumerated types (eg, enum XXX { AAA, BBB, CCC } ) They're
 using a keyword in common, but they're quite different concepts.


=20
 I think a sensible rule is that types (and templates that evaluate to
 types) start with a capital letter, while values (and functions/templat=




es

 that evaluate to values) are camelCased.
=20
 -Lars


=20
 The point is this: we do NOT have a style guide.
 We have consensus on a few things. Types start with a capital letter.
 Functions are camelCased. Many other things haven't actually been
 discussed and agreed to.
=20
 Note that simplistic rules are doomed to failure. For example, template
 aliases can be either values or types.
=20
 Also, any attempt to use precedent is a disaster since Phobos began as a
 complete mishmash of styles. In some cases people erroneously believed
 there was a convention, and attempted to adhere to it, even though no
 such convention existed.
=20
 We desperately need a style guide containing all the things which have
 actually been agreed to (and equally importantly, nothing which hasn't).
 The simplest thing to do would be to fix up the existing one on the
 website which nobody follows.


I've tried to get Andrei to agree to a style guide a few times, but he's=20
generally pushed back on it. I definitely think that we should have one if =
we=20
want to actually have a consistent style, but thus far, he hasn't agreed to=
=20
have one. I don't think that he's entirely against it, but we do keep runni=
ng=20
into issues where someone codes something one way and Andrei and/or others=
=20
expect it to be another and are unhappy that it is (e.g. the 80 character l=
ine=20
limit).

We don't need anything super strict or which specifies everything in insane=
=20
detail, but basic style guidelines would definitely be a good thing to have=
=2E=20
Maybe I should just put one together and post it on the Phobos list for=20
discussion.

=2D Jonathan M Davis
Mar 30 2011
parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 3/30/11 2:47 PM, Jonathan M Davis wrote:

 I've tried to get Andrei to agree to a style guide a few times, but he's
 generally pushed back on it. I definitely think that we should have one if we
 want to actually have a consistent style, but thus far, he hasn't agreed to
 have one.


I think that's not representing my viewpoint quite accurately, but 
getting to the bottom of whatever misunderstanding was there is not 
important.

It would be helpful to have a broad style guide. The existing one is a 
good start and is in fact already observed by much of Phobos (and in 
particular by most of my code). The problem with writing a more 
elaborate guide is finding the person(s) with the time and inclination 
to write a draft, get it vetted by the major contributors, and take it 
to completion.

For my money, just take the first that applies:

- Is it a function name? Use thisStyle.

- Is it a value (be it constant or variable)? Use thisStyle.

- Is it a type? Use ThisStyle.

- Is it a module name? Use this_style.

Beyond naming:

- Define variables as late as possible.

- Define constants and other names scoped appropriately.

- Prefer anonymous temporaries to named values within reason.

- Prefer ? : to if/else within reason.

- Prefer compact, effective code to verbose code within reason. Make 
every line count.

Nitpicks that 9 people have 10 opinions of:

- No 2+ empty lines please. Within a function, an empty line at best 
should be replaced by a comment describing the meaning of the next block.

- Try to fit functions loosely on one editor page.

- Brace on its own line. (In fact I don't care much either way, but this 
is the way the majority of the code is.) Note that this does cost more 
vertical space so it somewhat clashes with the previous point.

- Please avoid > 80 columns unless you feel it induces sterility in the 
long term.

- No tabs please.

- Comments should be high level (describing 3-10 lines) instead of 
low-level (describing the mechanics of the next line, which are already 
obvious in code).


Thanks,

Andrei
Mar 30 2011
next sibling parent reply Jonathan M Davis <jmdavisProg gmx.com> writes:
On 2011-03-30 13:07, Andrei Alexandrescu wrote:

 On 3/30/11 2:47 PM, Jonathan M Davis wrote:

 I've tried to get Andrei to agree to a style guide a few times, but he's
 generally pushed back on it. I definitely think that we should have one
 if we want to actually have a consistent style, but thus far, he hasn't
 agreed to have one.


 
 I think that's not representing my viewpoint quite accurately, but
 getting to the bottom of whatever misunderstanding was there is not
 important.
 
 It would be helpful to have a broad style guide. The existing one is a
 good start and is in fact already observed by much of Phobos (and in
 particular by most of my code). The problem with writing a more
 elaborate guide is finding the person(s) with the time and inclination
 to write a draft, get it vetted by the major contributors, and take it
 to completion.


I'll throw something together within the next day or two. It shouldn't take 
long. What'll be harder is getting people to actually read it and agree on it. 
Most discussions on this sort of thing in the past haven't generated much 
discussion even when people seem to definitely care (of course, the Phobos 
list seems pretty quiet of late, and the general activity for commits to 
Phobos from the Phobos dev team as a whole seems pretty low ever since we 
switched to git - whether that's the switch is related or not, I don't know). 
Still, we should get something going. The more that people are looking to 
contribute to Phobos, the clearer that we need to be on style requirements and 
guidelines.


 For my money, just take the first that applies:
 
 - Is it a function name? Use thisStyle.
 
 - Is it a value (be it constant or variable)? Use thisStyle.
 
 - Is it a type? Use ThisStyle.
 
 - Is it a module name? Use this_style.


This would be a good example of something that's never been agreed on AFAIK. 
Does any module in Phobos use this_style? I'm not necessarily opposed, but I 
don't think that it's been used yet.


 Beyond naming:
 
 - Define variables as late as possible.
 
 - Define constants and other names scoped appropriately.
 
 - Prefer anonymous temporaries to named values within reason.
 
 - Prefer ? : to if/else within reason.
 
 - Prefer compact, effective code to verbose code within reason. Make
 every line count.
 
 Nitpicks that 9 people have 10 opinions of:
 
 - No 2+ empty lines please. Within a function, an empty line at best
 should be replaced by a comment describing the meaning of the next block.
 
 - Try to fit functions loosely on one editor page.
 
 - Brace on its own line. (In fact I don't care much either way, but this
 is the way the majority of the code is.) Note that this does cost more
 vertical space so it somewhat clashes with the previous point.
 
 - Please avoid > 80 columns unless you feel it induces sterility in the
 long term.
 
 - No tabs please.
 
 - Comments should be high level (describing 3-10 lines) instead of
 low-level (describing the mechanics of the next line, which are already
 obvious in code).


Some of these are certainly debatable, but are overall good. We can discuss it 
as part of reviewing an actual style document though.

- Jonathan M Davis
Mar 30 2011
parent KennyTM~ <kennytm gmail.com> writes:
On Mar 31, 11 04:35, Jonathan M Davis wrote:

  For my money, just take the first that applies:

  - Is it a function name? Use thisStyle.

  - Is it a value (be it constant or variable)? Use thisStyle.

  - Is it a type? Use ThisStyle.

  - Is it a module name? Use this_style.




 This would be a good example of something that's never been agreed on AFAIK.
 Does any module in Phobos use this_style? I'm not necessarily opposed, but I
 don't think that it's been used yet.


It is more like 'thisstyle'

std.
    bigint
    bitmanip
    datetime
    getopt
    outbuffer
    outofmemory   // <-- btw why is it thing still documented?
    socketstream
    typecons
    typetuple
Mar 30 2011
prev sibling next sibling parent reply bearophile <bearophileHUGS lycos.com> writes:
Andrei:


 Beyond naming:


Some standard for member attributes? Like m_something, etc? I don't like this a
lot, but having a style guide on this too is useful for Phobos (and user code).

Bye,
bearophile
Mar 30 2011
next sibling parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 3/30/11 5:24 PM, bearophile wrote:

 Andrei:


 Beyond naming:


 Some standard for member attributes? Like m_something, etc? I don't like this
a lot, but having a style guide on this too is useful for Phobos (and user
code).

 Bye,
 bearophile


I usually prepend private data members with an underscore.

Andrei
Mar 30 2011
parent reply Jonathan M Davis <jmdavisProg gmx.com> writes:
On 2011-03-30 15:27, Andrei Alexandrescu wrote:

 On 3/30/11 5:24 PM, bearophile wrote:

 Andrei:

 Beyond naming:


 Some standard for member attributes? Like m_something, etc? I don't like
 this a lot, but having a style guide on this too is useful for Phobos
 (and user code).
 
 Bye,
 bearophile


 
 I usually prepend private data members with an underscore.


That's what std.datetime and std.file do. Regardless, member variables often 
need a different naming scheme from normal variables thanks to properties that 
have the same name. That can go in the style guide, but I don't think that the 
naming scheme for private member variables or local variables is as important 
as the public ones, since they're part of the API. It would still be good to 
be consistent (like it's good to be consistent with braces), but when it comes 
to names, the primary concern is the public API.

- Jonathan M Davis
Mar 30 2011
parent Jacob Carlborg <doob me.com> writes:
On 2011-03-31 01:12, Jonathan M Davis wrote:

 On 2011-03-30 15:27, Andrei Alexandrescu wrote:

 On 3/30/11 5:24 PM, bearophile wrote:

 Andrei:

 Beyond naming:


 Some standard for member attributes? Like m_something, etc? I don't like
 this a lot, but having a style guide on this too is useful for Phobos
 (and user code).

 Bye,
 bearophile


 I usually prepend private data members with an underscore.


 That's what std.datetime and std.file do. Regardless, member variables often
 need a different naming scheme from normal variables thanks to properties that
 have the same name. That can go in the style guide, but I don't think that the
 naming scheme for private member variables or local variables is as important
 as the public ones, since they're part of the API. It would still be good to
 be consistent (like it's good to be consistent with braces), but when it comes
 to names, the primary concern is the public API.

 - Jonathan M Davis


I think this is only valid if the member variables have a corresponding 
getter/setter with the same name.

-- 
/Jacob Carlborg
Mar 31 2011
prev sibling parent reply dsimcha <dsimcha yahoo.com> writes:
On 3/30/2011 6:24 PM, bearophile wrote:

 Andrei:


 Beyond naming:


 Some standard for member attributes? Like m_something, etc? I don't like this
a lot, but having a style guide on this too is useful for Phobos (and user
code).

 Bye,
 bearophile


I think the style guide should be focused mostly (though not 
exclusively) on things that affect the public interface.  Things like 
naming conventions for private member variables are minor details and 
not worth being bureaucratic about.
Mar 30 2011
parent reply Daniel Gibson <metalcaedes gmail.com> writes:
Am 31.03.2011 01:57, schrieb dsimcha:

 On 3/30/2011 6:24 PM, bearophile wrote:

 Andrei:


 Beyond naming:


 Some standard for member attributes? Like m_something, etc? I don't
 like this a lot, but having a style guide on this too is useful for
 Phobos (and user code).

 Bye,
 bearophile


 I think the style guide should be focused mostly (though not
 exclusively) on things that affect the public interface. Things like
 naming conventions for private member variables are minor details and
 not worth being bureaucratic about.


Well, I remember a lengthy discussion on the maximum line length in 
Phobos, so it seems like stuff that isn't visible from the public 
interface still seems to bother people (including Andrei).
Mar 30 2011
parent Jonathan M Davis <jmdavisProg gmx.com> writes:
On 2011-03-30 17:17, Daniel Gibson wrote:

 Am 31.03.2011 01:57, schrieb dsimcha:

 On 3/30/2011 6:24 PM, bearophile wrote:

 Andrei:

 Beyond naming:


 Some standard for member attributes? Like m_something, etc? I don't
 like this a lot, but having a style guide on this too is useful for
 Phobos (and user code).
 
 Bye,
 bearophile


 
 I think the style guide should be focused mostly (though not
 exclusively) on things that affect the public interface. Things like
 naming conventions for private member variables are minor details and
 not worth being bureaucratic about.


 
 Well, I remember a lengthy discussion on the maximum line length in
 Phobos, so it seems like stuff that isn't visible from the public
 interface still seems to bother people (including Andrei).


If you look at Andrei's post in this thread where he lists the beginnings of a 
possible style guide, _most_ of it is stuff that's not part of the public API.

- Jonathan M Davis
Mar 30 2011
prev sibling parent reply Jacob Carlborg <doob me.com> writes:
On 3/30/11 10:07 PM, Andrei Alexandrescu wrote:

 On 3/30/11 2:47 PM, Jonathan M Davis wrote:

 I've tried to get Andrei to agree to a style guide a few times, but he's
 generally pushed back on it. I definitely think that we should have
 one if we
 want to actually have a consistent style, but thus far, he hasn't
 agreed to
 have one.


 I think that's not representing my viewpoint quite accurately, but
 getting to the bottom of whatever misunderstanding was there is not
 important.

 It would be helpful to have a broad style guide. The existing one is a
 good start and is in fact already observed by much of Phobos (and in
 particular by most of my code). The problem with writing a more
 elaborate guide is finding the person(s) with the time and inclination
 to write a draft, get it vetted by the major contributors, and take it
 to completion.


Which one is the existing one, it it available on the DigitalMars site?


 For my money, just take the first that applies:

 - Is it a function name? Use thisStyle.

 - Is it a value (be it constant or variable)? Use thisStyle.

 - Is it a type? Use ThisStyle.

 - Is it a module name? Use this_style.

 Beyond naming:

 - Define variables as late as possible.

 - Define constants and other names scoped appropriately.

 - Prefer anonymous temporaries to named values within reason.

 - Prefer ? : to if/else within reason.

 - Prefer compact, effective code to verbose code within reason. Make
 every line count.


I don't like this one. I would say prefer readable code to compact code. 
Don't be afraid of having long descriptive names of variables and having 
vertical space in your code. I don't mean you should put three newlines 
between two function declarations but I usually put a newline before and 
after statements:

int a;
int b;

foo();
bar();

if (a == b)
    foo();

else
    bar();

int c = 3;


 Nitpicks that 9 people have 10 opinions of:

 - No 2+ empty lines please. Within a function, an empty line at best
 should be replaced by a comment describing the meaning of the next block.

 - Try to fit functions loosely on one editor page.

 - Brace on its own line. (In fact I don't care much either way, but this
 is the way the majority of the code is.) Note that this does cost more
 vertical space so it somewhat clashes with the previous point.

 - Please avoid > 80 columns unless you feel it induces sterility in the
 long term.


Do you know how narrow 80 columns look on a wide screen. I think it 
looks narrow on a regular (non-wide) screen.


 - No tabs please.

 - Comments should be high level (describing 3-10 lines) instead of
 low-level (describing the mechanics of the next line, which are already
 obvious in code).


 Thanks,

 Andrei



-- 
/Jacob Carlborg
Mar 31 2011
parent Jonathan M Davis <jmdavisProg gmx.com> writes:
On 2011-03-31 00:09, Jacob Carlborg wrote:

 On 3/30/11 10:07 PM, Andrei Alexandrescu wrote:

 On 3/30/11 2:47 PM, Jonathan M Davis wrote:

 I've tried to get Andrei to agree to a style guide a few times, but he's
 generally pushed back on it. I definitely think that we should have
 one if we
 want to actually have a consistent style, but thus far, he hasn't
 agreed to
 have one.


 
 I think that's not representing my viewpoint quite accurately, but
 getting to the bottom of whatever misunderstanding was there is not
 important.
 
 It would be helpful to have a broad style guide. The existing one is a
 good start and is in fact already observed by much of Phobos (and in
 particular by most of my code). The problem with writing a more
 elaborate guide is finding the person(s) with the time and inclination
 to write a draft, get it vetted by the major contributors, and take it
 to completion.


 
 Which one is the existing one, it it available on the DigitalMars site?
 

 For my money, just take the first that applies:
 
 - Is it a function name? Use thisStyle.
 
 - Is it a value (be it constant or variable)? Use thisStyle.
 
 - Is it a type? Use ThisStyle.
 
 - Is it a module name? Use this_style.
 
 Beyond naming:
 
 - Define variables as late as possible.
 
 - Define constants and other names scoped appropriately.
 
 - Prefer anonymous temporaries to named values within reason.
 
 - Prefer ? : to if/else within reason.
 
 - Prefer compact, effective code to verbose code within reason. Make
 every line count.


 
 I don't like this one. I would say prefer readable code to compact code.
 Don't be afraid of having long descriptive names of variables and having
 vertical space in your code. I don't mean you should put three newlines
 between two function declarations but I usually put a newline before and
 after statements:
 
 int a;
 int b;
 
 foo();
 bar();
 
 if (a == b)
     foo();
 
 else
     bar();
 
 int c = 3;
 

 Nitpicks that 9 people have 10 opinions of:
 
 - No 2+ empty lines please. Within a function, an empty line at best
 should be replaced by a comment describing the meaning of the next block.
 
 - Try to fit functions loosely on one editor page.
 
 - Brace on its own line. (In fact I don't care much either way, but this
 is the way the majority of the code is.) Note that this does cost more
 vertical space so it somewhat clashes with the previous point.
 
 - Please avoid > 80 columns unless you feel it induces sterility in the
 long term.


 
 Do you know how narrow 80 columns look on a wide screen. I think it
 looks narrow on a regular (non-wide) screen.


I definitely have to agree with you on that one (most everything you said 
actually), but there are obviously differing opinions and it needs to be 
discussed. I've put together an initial proposal for a style guide (based on 
what Phobos has been doing and what's been discussed on the newsgroup) and 
posted it to the Phobos list. So, discuss away on that thread. Hopefully, we 
can come to a reasonable consensus.

- Jonathan M Davis
Mar 31 2011
prev sibling parent Jonathan M Davis <jmdavisProg gmx.com> writes:
On 2011-03-30 04:23, Don wrote:

 Jonathan M Davis wrote:

 On 2011-03-30 01:27, Jacob Carlborg wrote:

 On 3/30/11 1:30 AM, Jesse Phillips wrote:

 Jacob Carlborg Wrote:

 I've made a few minor changes:
 
 * Renamed EmailStatusCode.Off ->  None and On ->  Any
 * Added and clarified the documentation for EmailStatusCode.Any and
 None * Updated the documentation
 
 Github: https://github.com/jacob-carlborg/phobos/tree/isemail
 Docs: http://dl.dropbox.com/u/18386187/isemail.html
 
 --
 /Jacob Carlborg


 
 I believe enum values are to be named lowercase first.
 EmailStatusCode.any


 
 I don't know what the style guide says about enum members but if that's
 the case I'll change the names to begin with lowercase.


 
 All names are camelcased.


 
 That's not true. ALLCAPS is relatively common in Phobos.
 There is absolutely no way PI is going to become pi.
 

 All type names begin with an uppercase letter, and
 all variables begin with a lowercase letter (with the possible exception
 of private member variables beginning with _ - but what's private to a
 class or struct isn't as critical as the public API regardless).


 
 That part is clear.
 
  > enum values fall in the same camp as variables.
 
 I never heard that before, and it doesn't seem to be true throughout
 Phobos. Grepping for all enum declarations (there isn't very many of
 them actually), I found some which were like that, some which start with
 uppercase, and some which are all caps.
 
 I think you're assuming more concensus on style than has ever actually
 been discussed.


I'd have to go digging through the mailing list to find posts on it, but 
Andrei made it clear that that was what was the intended naming convention and 
pretty much all of the newer stuff in Phobos follows that convention. Older 
stuff definitely doesn't follow that convention, but the newer stuff does. In 
particular, all caps was viewed as bad for enums because of how often they get 
used in D, and having variables which are all caps used frequently is 
ugly/annoying. I don't think that there was a huge discussion about it, and I 
believe that the push for the style I described came primarily from Andrei, 
but what discussion that there was agreed on treating enum values identically 
to normal variables as far as their naming coventions go. And all of the newer 
stuff follows that convention. And since you have to use the enum's name with 
all of the enum values anyway, it's not like you need a drastic difference in 
naming to distinguish enums other than manifest constants.

- Jonathan M Davis
Mar 30 2011
prev sibling parent Jacob Carlborg <doob me.com> writes:
On 3/30/11 10:49 AM, Jonathan M Davis wrote:

 On 2011-03-30 01:27, Jacob Carlborg wrote:

 On 3/30/11 1:30 AM, Jesse Phillips wrote:

 Jacob Carlborg Wrote:

 I've made a few minor changes:

 * Renamed EmailStatusCode.Off ->   None and On ->   Any
 * Added and clarified the documentation for EmailStatusCode.Any and None
 * Updated the documentation

 Github: https://github.com/jacob-carlborg/phobos/tree/isemail
 Docs: http://dl.dropbox.com/u/18386187/isemail.html

 --
 /Jacob Carlborg


 I believe enum values are to be named lowercase first.
 EmailStatusCode.any


 I don't know what the style guide says about enum members but if that's
 the case I'll change the names to begin with lowercase.


 All names are camelcased. All type names begin with an uppercase letter, and
 all variables begin with a lowercase letter (with the possible exception of
 private member variables beginning with _ - but what's private to a class or
 struct isn't as critical as the public API regardless). enum values fall in
 the same camp as variables.

 We don't really have an actual document anywhere that lays out the style.
 Andrei has resisted the idea, though I don't think that he's entirely against
 it, so unfortunately, I don't think that there's currently anywhere that you
 can look it up. Discussions of the newsgroups have made the decisions on the
 naming conventions clear though, at least as long as you've paid attention and
 remember them, which obviously isn't going to happen for newer folks on the
 list, and unless you're regularly writing Phobos code and making sure that you
 follow the appropriate conventions, it's probably not all that hard to forget
 them if they're not what you'd do anyway.

 In any case, enum values follow the same style as normal variables -
 camelcased, beginning with a lowercase letter.

 - Jonathan M Davis


Ok, then I'll change to that.

-- 
/Jacob Carlborg
Mar 30 2011
prev sibling next sibling parent Jonathan M Davis <jmdavisProg gmx.com> writes:
On 2011-03-29 14:06, Jacob Carlborg wrote:

 I've made a few minor changes:
 
 * Renamed EmailStatusCode.Off -> None and On -> Any
 * Added and clarified the documentation for EmailStatusCode.Any and None
 * Updated the documentation
 
 Github: https://github.com/jacob-carlborg/phobos/tree/isemail
 Docs: http://dl.dropbox.com/u/18386187/isemail.html


Shouldn't isEmail have a template constraint which verifies that the e-mail 
variable is a valid string type? Also, it would probably be better to use Char 
instead of T for the type. It's clearer that way. I believe that that's what 
std.string does.

I'd strongly consider making EmailStatus' public member variables private with 
getter properties (no setters obviously, since they're const), since you can't 
even assign to a const struct, and that could be annoying. For instance, you 
can't really put them in an array very well. By making them only have getter 
properties, you get the same effect but can still put the struct in arrays or 
reassign it if you need to. const member variables in classes isn't such a big 
deal, but I'm disinclined to use them in structs due to the issues that they 
cause.

I'd be inclined to make EmailStatus' constructor private. I'm not sure that it 
makes any sense for it to be public. Why would anyone create one rather than 
using isEmail to create it?

You should probably add a note to the documentation that the DNS check hasn't 
been implemented yet.

It would probably be a good idea to create a free function which takes an 
EmailStatusCode and returns the status string, move the logic in the status 
function to there, and have it call the new function. That way, if a program 
needed to know what the actual string was without having an e-mail with that 
particular error, it could get at the string.

If you're going to mention a version number, you might want to make it clearer 
what the code is based on, since from Phobos' perspective, the version number 
makes no sense. It _does_ make sense to say that it's based on version X of 
library Y but not that the module itself is version X - especially when that 
version is 3+, and it's brand-new.



Nitpicks:

You have tabs and trailing space in the file. You shouldn't have either. And 
even if we wanted tabs, they were used inconsistently.

Why did you create aliases for front, back, and popFront? That's a really odd 
thing to do and makes the code harder to understand. I would have considered 
that to fall in the "useless alias" category and best to be avoided.

As stated before, all of your enums seem to start with a capital letter when 
the typical thing to do in Phobos at this point is to start with a lowercase 
letter.

It would be nice if isEmail were broken up given its size, but it does seem 
like the sort of function which wouldn't break up very easily. The most 
obvious way to do that would be to create separate functions for each case 
statement, but given all of the variables involved, that wouldn't work very 
well. About the only sane way that I could think of doing that would be to 
create class or struct which held all of the logic and had all of those local 
variables as member variables. Then each of the cases could be broken up into 
separate functions. It would probably be declared internal to isEmail, and 
isEmail would construct one and then get the result out of it. That would 
allow you to break up the function. However, I'm not at all convinced that 
that would really be a better solution. It does seem a bit convoluted to do 
that just to break the function up. If it really helped the function's 
readability and maintainability then it would be worth it, but it might make 
it worse. So, I don't really like the size of the function, and that's a way 
that you could break it up, but unless you really think that that's a good 
idea, I'd just leave it as is. It's the sort of function that's generally 
stuck being long.



Overall, it looks pretty good. And as long as it works, it should be fine 
other than the few API issues that I mentioned, and they're not exactly 
enormous issues.

- Jonathan M Davis
Apr 01 2011
prev sibling next sibling parent reply Jonathan M Davis <jmdavisProg gmx.com> writes:
On 2011-03-29 14:06, Jacob Carlborg wrote:

 I've made a few minor changes:
 
 * Renamed EmailStatusCode.Off -> None and On -> Any
 * Added and clarified the documentation for EmailStatusCode.Any and None
 * Updated the documentation
 
 Github: https://github.com/jacob-carlborg/phobos/tree/isemail
 Docs: http://dl.dropbox.com/u/18386187/isemail.html


Shouldn't isEmail have a template constraint which verifies that the e-mail 
variable is a valid string type? Also, it would probably be better to use Char 
instead of T for the type. It's clearer that way. I believe that that's what 
std.string does.

I'd strongly consider making EmailStatus' public member variables private with 
getter properties (no setters obviously, since they're const), since you can't 
even assign to a const struct, and that could be annoying. For instance, you 
can't really put them in an array very well. By making them only have getter 
properties, you get the same effect but can still put the struct in arrays or 
reassign it if you need to. const member variables in classes isn't such a big 
deal, but I'm disinclined to use them in structs due to the issues that they 
cause.

I'd be inclined to make EmailStatus' constructor private. I'm not sure that it 
makes any sense for it to be public. Why would anyone create one rather than 
using isEmail to create it?

You should probably add a note to the documentation that the DNS check hasn't 
been implemented yet.

It would probably be a good idea to create a free function which takes an 
EmailStatusCode and returns the status string, move the logic in the status 
function to there, and have it call the new function. That way, if a program 
needed to know what the actual string was without having an e-mail with that 
particular error, it could get at the string.

If you're going to mention a version number, you might want to make it clearer 
what the code is based on, since from Phobos' perspective, the version number 
makes no sense. It _does_ make sense to say that it's based on version X of 
library Y but not that the module itself is version X - especially when that 
version is 3+, and it's brand-new.



Nitpicks:

You have tabs and trailing space in the file. You shouldn't have either. And 
even if we wanted tabs, they were used inconsistently.

Why did you create aliases for front, back, and popFront? That's a really odd 
thing to do and makes the code harder to understand. I would have considered 
that to fall in the "useless alias" category and best to be avoided.

As stated before, all of your enums seem to start with a capital letter when 
the typical thing to do in Phobos at this point is to start with a lowercase 
letter.

It would be nice if isEmail were broken up given its size, but it does seem 
like the sort of function which wouldn't break up very easily. The most 
obvious way to do that would be to create separate functions for each case 
statement, but given all of the variables involved, that wouldn't work very 
well. About the only sane way that I could think of doing that would be to 
create class or struct which held all of the logic and had all of those local 
variables as member variables. Then each of the cases could be broken up into 
separate functions. It would probably be declared internal to isEmail, and 
isEmail would construct one and then get the result out of it. That would 
allow you to break up the function. However, I'm not at all convinced that 
that would really be a better solution. It does seem a bit convoluted to do 
that just to break the function up. If it really helped the function's 
readability and maintainability then it would be worth it, but it might make 
it worse. So, I don't really like the size of the function, and that's a way 
that you could break it up, but unless you really think that that's a good 
idea, I'd just leave it as is. It's the sort of function that's generally 
stuck being long.



Overall, it looks pretty good. And as long as it works, it should be fine 
other than the few API issues that I mentioned, and they're not exactly 
enormous issues.

- Jonathan M Davis
Apr 01 2011
parent reply Jacob Carlborg <doob me.com> writes:
On 2011-04-01 11:01, Jonathan M Davis wrote:

 On 2011-03-29 14:06, Jacob Carlborg wrote:

 I've made a few minor changes:

 * Renamed EmailStatusCode.Off ->  None and On ->  Any
 * Added and clarified the documentation for EmailStatusCode.Any and None
 * Updated the documentation

 Github: https://github.com/jacob-carlborg/phobos/tree/isemail
 Docs: http://dl.dropbox.com/u/18386187/isemail.html


 Shouldn't isEmail have a template constraint which verifies that the e-mail
 variable is a valid string type? Also, it would probably be better to use Char
 instead of T for the type. It's clearer that way. I believe that that's what
 std.string does.


That's a good idea.


 I'd strongly consider making EmailStatus' public member variables private with
 getter properties (no setters obviously, since they're const), since you can't
 even assign to a const struct, and that could be annoying. For instance, you
 can't really put them in an array very well. By making them only have getter
 properties, you get the same effect but can still put the struct in arrays or
 reassign it if you need to. const member variables in classes isn't such a big
 deal, but I'm disinclined to use them in structs due to the issues that they
 cause.


Ok, didn't think about that.


 I'd be inclined to make EmailStatus' constructor private. I'm not sure that it
 makes any sense for it to be public. Why would anyone create one rather than
 using isEmail to create it?


I guess you're right.


 You should probably add a note to the documentation that the DNS check hasn't
 been implemented yet.


Yes.


 It would probably be a good idea to create a free function which takes an
 EmailStatusCode and returns the status string, move the logic in the status
 function to there, and have it call the new function. That way, if a program
 needed to know what the actual string was without having an e-mail with that
 particular error, it could get at the string.


Is this really necessary? Would you ever have the status code without 
the EmailStatus?


 If you're going to mention a version number, you might want to make it clearer
 what the code is based on, since from Phobos' perspective, the version number
 makes no sense. It _does_ make sense to say that it's based on version X of
 library Y but not that the module itself is version X - especially when that
 version is 3+, and it's brand-new.


Yeah, I kind of most of that text as is.


 Nitpicks:

 You have tabs and trailing space in the file. You shouldn't have either. And
 even if we wanted tabs, they were used inconsistently.


Forgot about converting the tabs to spaces? How can I easily find 
trailing spaces?


 Why did you create aliases for front, back, and popFront? That's a really odd
 thing to do and makes the code harder to understand. I would have considered
 that to fall in the "useless alias" category and best to be avoided.


I like those names better and though it wouldn't matter all the much 
since they're private.


 As stated before, all of your enums seem to start with a capital letter when
 the typical thing to do in Phobos at this point is to start with a lowercase
 letter.


Yes, didn't know what the naming convention were for enum members when I 
started.


 It would be nice if isEmail were broken up given its size, but it does seem
 like the sort of function which wouldn't break up very easily. The most
 obvious way to do that would be to create separate functions for each case
 statement, but given all of the variables involved, that wouldn't work very
 well. About the only sane way that I could think of doing that would be to
 create class or struct which held all of the logic and had all of those local
 variables as member variables. Then each of the cases could be broken up into
 separate functions. It would probably be declared internal to isEmail, and
 isEmail would construct one and then get the result out of it. That would
 allow you to break up the function. However, I'm not at all convinced that
 that would really be a better solution. It does seem a bit convoluted to do
 that just to break the function up. If it really helped the function's
 readability and maintainability then it would be worth it, but it might make
 it worse. So, I don't really like the size of the function, and that's a way
 that you could break it up, but unless you really think that that's a good
 idea, I'd just leave it as is. It's the sort of function that's generally
 stuck being long.


I agree with you, it's far too big but I don't know how I could break it 
up. It would be a lot easier if I've written the function myself but 
it's just a port of a PHP function.


 Overall, it looks pretty good. And as long as it works, it should be fine
 other than the few API issues that I mentioned, and they're not exactly
 enormous issues.


Thanks.


 - Jonathan M Davis



-- 
/Jacob Carlborg
Apr 02 2011
parent reply Jonathan M Davis <jmdavisProg gmx.com> writes:
On 2011-04-02 09:26, Jacob Carlborg wrote:

 On 2011-04-01 11:01, Jonathan M Davis wrote:

 On 2011-03-29 14:06, Jacob Carlborg wrote:

 I've made a few minor changes:
 
 * Renamed EmailStatusCode.Off ->  None and On ->  Any
 * Added and clarified the documentation for EmailStatusCode.Any and None
 * Updated the documentation
 
 Github: https://github.com/jacob-carlborg/phobos/tree/isemail
 Docs: http://dl.dropbox.com/u/18386187/isemail.html


 
 Shouldn't isEmail have a template constraint which verifies that the
 e-mail variable is a valid string type? Also, it would probably be
 better to use Char instead of T for the type. It's clearer that way. I
 believe that that's what std.string does.


 
 That's a good idea.


I'd say that pretty much every template should have a template constraint on 
it. I'm not sure that I've ever seen a template which was flexible enough that 
it would compile with anything you gave it, and if there's an argument that 
you can give it which won't compile with it, it should fail the template 
constraints, otherwise the error messags are much harder to understand and 
track down.


 I'd strongly consider making EmailStatus' public member variables private
 with getter properties (no setters obviously, since they're const),
 since you can't even assign to a const struct, and that could be
 annoying. For instance, you can't really put them in an array very well.
 By making them only have getter properties, you get the same effect but
 can still put the struct in arrays or reassign it if you need to. const
 member variables in classes isn't such a big deal, but I'm disinclined
 to use them in structs due to the issues that they cause.


 
 Ok, didn't think about that.
 

 I'd be inclined to make EmailStatus' constructor private. I'm not sure
 that it makes any sense for it to be public. Why would anyone create one
 rather than using isEmail to create it?


 
 I guess you're right.
 

 You should probably add a note to the documentation that the DNS check
 hasn't been implemented yet.


 
 Yes.
 

 It would probably be a good idea to create a free function which takes an
 EmailStatusCode and returns the status string, move the logic in the
 status function to there, and have it call the new function. That way,
 if a program needed to know what the actual string was without having an
 e-mail with that particular error, it could get at the string.


 
 Is this really necessary? Would you ever have the status code without
 the EmailStatus?


In most cases, no. However, I can easily believe that a fancier program would 
care. For instance, anything which wanted to list the various e-mail status 
codes and their associated strings would need to have the list of 
EmailStatusCodes and their associated strings.

I don't doubt that few programs will care. But it would be so simple to make 
it possible to get a string and from its corresponding status code without 
having an EmailStatus, that I see no reason to not make it possible to do so. 
It makes the module more flexible, and I expect that if the module is used 
enough, then a program at some point will want that capability.


 If you're going to mention a version number, you might want to make it
 clearer what the code is based on, since from Phobos' perspective, the
 version number makes no sense. It _does_ make sense to say that it's
 based on version X of library Y but not that the module itself is
 version X - especially when that version is 3+, and it's brand-new.


 
 Yeah, I kind of most of that text as is.
 

 Nitpicks:
 
 You have tabs and trailing space in the file. You shouldn't have either.
 And even if we wanted tabs, they were used inconsistently.


 
 Forgot about converting the tabs to spaces? How can I easily find
 trailing spaces?


We have only been using spaces in Phobos - no tabs - and I don't expect that 
to change, so code in Phobos shouldn't have tabs in it. Mixing tabs and spaces 
tends to ruin formatting and cause problems. And trailing spaces are just 
wasted characters.

Personally, I have vim set up to always highlight any tabs as well as trailing 
whitespace, so I notice it immediately - which is why I noticed in this case.

Typically, there's a way to get code editors to show extra whitespace and/or 
remove it when saving the file. They also typically have a way to convert tabs 
to spaces. How that works depends in your editor though depends on your 
editor.

If you're using vim, then :retab would turn the tabs into spaces and the 
command :s/\s\+$// will remove all trailing whitespace (and presumably that 
can be converted into an appropriate sed command, but I'm not well-versed in 
sed).


 Why did you create aliases for front, back, and popFront? That's a really
 odd thing to do and makes the code harder to understand. I would have
 considered that to fall in the "useless alias" category and best to be
 avoided.


 
 I like those names better and though it wouldn't matter all the much
 since they're private.


It doesn't matter as much, but I think that it still matters. Using 
unnecessary aliases makes it harder to read code - especially when they're 
aliases for well-known symbols. front, popFront, and back are standard Phobos 
functions which are part of one of the core concepts in Phobos, and they're 
what the rest of Phobos uses. You can't get much more standard than that. 
tstring at least makes the code shorter, and it's actually listed before it's 
used (unlike the other aliases), and I'm still torn on whether it should be 
there. But aliases which are simple renamings for standard Phobos functions 
are completely unnecessary and harm code maintainability for anyone else who 
reads the code.

- Jonathan M Davis
Apr 02 2011
parent reply Jacob Carlborg <doob me.com> writes:
On 2011-04-02 23:33, Jonathan M Davis wrote:

 On 2011-04-02 09:26, Jacob Carlborg wrote:

 On 2011-04-01 11:01, Jonathan M Davis wrote:

 On 2011-03-29 14:06, Jacob Carlborg wrote:

 I've made a few minor changes:

 * Renamed EmailStatusCode.Off ->   None and On ->   Any
 * Added and clarified the documentation for EmailStatusCode.Any and None
 * Updated the documentation

 Github: https://github.com/jacob-carlborg/phobos/tree/isemail
 Docs: http://dl.dropbox.com/u/18386187/isemail.html


 Shouldn't isEmail have a template constraint which verifies that the
 e-mail variable is a valid string type? Also, it would probably be
 better to use Char instead of T for the type. It's clearer that way. I
 believe that that's what std.string does.


 That's a good idea.


 I'd say that pretty much every template should have a template constraint on
 it. I'm not sure that I've ever seen a template which was flexible enough that
 it would compile with anything you gave it, and if there's an argument that
 you can give it which won't compile with it, it should fail the template
 constraints, otherwise the error messags are much harder to understand and
 track down.


I've added a static assert with a custom error message. I think  static 
asserts can give better error messages than template constraints.


 I'd strongly consider making EmailStatus' public member variables private
 with getter properties (no setters obviously, since they're const),
 since you can't even assign to a const struct, and that could be
 annoying. For instance, you can't really put them in an array very well.
 By making them only have getter properties, you get the same effect but
 can still put the struct in arrays or reassign it if you need to. const
 member variables in classes isn't such a big deal, but I'm disinclined
 to use them in structs due to the issues that they cause.


 Ok, didn't think about that.


 I'd be inclined to make EmailStatus' constructor private. I'm not sure
 that it makes any sense for it to be public. Why would anyone create one
 rather than using isEmail to create it?


 I guess you're right.


 You should probably add a note to the documentation that the DNS check
 hasn't been implemented yet.


 Yes.


 It would probably be a good idea to create a free function which takes an
 EmailStatusCode and returns the status string, move the logic in the
 status function to there, and have it call the new function. That way,
 if a program needed to know what the actual string was without having an
 e-mail with that particular error, it could get at the string.


 Is this really necessary? Would you ever have the status code without
 the EmailStatus?


 In most cases, no. However, I can easily believe that a fancier program would
 care. For instance, anything which wanted to list the various e-mail status
 codes and their associated strings would need to have the list of
 EmailStatusCodes and their associated strings.

 I don't doubt that few programs will care. But it would be so simple to make
 it possible to get a string and from its corresponding status code without
 having an EmailStatus, that I see no reason to not make it possible to do so.
 It makes the module more flexible, and I expect that if the module is used
 enough, then a program at some point will want that capability.


Ok, I'll add a function, any idea for a good name?


 If you're going to mention a version number, you might want to make it
 clearer what the code is based on, since from Phobos' perspective, the
 version number makes no sense. It _does_ make sense to say that it's
 based on version X of library Y but not that the module itself is
 version X - especially when that version is 3+, and it's brand-new.


 Yeah, I kind of most of that text as is.


 Nitpicks:

 You have tabs and trailing space in the file. You shouldn't have either.
 And even if we wanted tabs, they were used inconsistently.


 Forgot about converting the tabs to spaces? How can I easily find
 trailing spaces?


 We have only been using spaces in Phobos - no tabs - and I don't expect that
 to change, so code in Phobos shouldn't have tabs in it. Mixing tabs and spaces
 tends to ruin formatting and cause problems. And trailing spaces are just
 wasted characters.

 Personally, I have vim set up to always highlight any tabs as well as trailing
 whitespace, so I notice it immediately - which is why I noticed in this case.

 Typically, there's a way to get code editors to show extra whitespace and/or
 remove it when saving the file. They also typically have a way to convert tabs
 to spaces. How that works depends in your editor though depends on your
 editor.

 If you're using vim, then :retab would turn the tabs into spaces and the
 command :s/\s\+$// will remove all trailing whitespace (and presumably that
 can be converted into an appropriate sed command, but I'm not well-versed in
 sed).


I think it's easier to work with tabs than spaces so I planned to work 
with tabs and than the last thing I would do before submitting a pull 
request would be to convert the tabs spaces, just forgot that. I just 
did a simple search and replace in my editor with a regular expression 
to remove the trailing spaces.


 Why did you create aliases for front, back, and popFront? That's a really
 odd thing to do and makes the code harder to understand. I would have
 considered that to fall in the "useless alias" category and best to be
 avoided.


 I like those names better and though it wouldn't matter all the much
 since they're private.


 It doesn't matter as much, but I think that it still matters. Using
 unnecessary aliases makes it harder to read code - especially when they're
 aliases for well-known symbols. front, popFront, and back are standard Phobos
 functions which are part of one of the core concepts in Phobos, and they're
 what the rest of Phobos uses. You can't get much more standard than that.
 tstring at least makes the code shorter, and it's actually listed before it's
 used (unlike the other aliases), and I'm still torn on whether it should be
 there. But aliases which are simple renamings for standard Phobos functions
 are completely unnecessary and harm code maintainability for anyone else who
 reads the code.


I've already removed these aliases (not tstring).


 - Jonathan M Davis



-- 
/Jacob Carlborg
Apr 03 2011
parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 4/3/11 6:38 AM, Jacob Carlborg wrote:

 On 2011-04-02 23:33, Jonathan M Davis wrote:

 On 2011-04-02 09:26, Jacob Carlborg wrote:

 On 2011-04-01 11:01, Jonathan M Davis wrote:

 On 2011-03-29 14:06, Jacob Carlborg wrote:

 I've made a few minor changes:

 * Renamed EmailStatusCode.Off -> None and On -> Any
 * Added and clarified the documentation for EmailStatusCode.Any and
 None
 * Updated the documentation

 Github: https://github.com/jacob-carlborg/phobos/tree/isemail
 Docs: http://dl.dropbox.com/u/18386187/isemail.html


 Shouldn't isEmail have a template constraint which verifies that the
 e-mail variable is a valid string type? Also, it would probably be
 better to use Char instead of T for the type. It's clearer that way. I
 believe that that's what std.string does.


 That's a good idea.


 I'd say that pretty much every template should have a template
 constraint on
 it. I'm not sure that I've ever seen a template which was flexible
 enough that
 it would compile with anything you gave it, and if there's an argument
 that
 you can give it which won't compile with it, it should fail the template
 constraints, otherwise the error messags are much harder to understand
 and
 track down.


 I've added a static assert with a custom error message. I think static
 asserts can give better error messages than template constraints.


Please use a template constraint. This is the right thing to do and we 
use it throughout Phobos.

Thanks,

Andrei
Apr 03 2011
next sibling parent "Robert Jacques" <sandford jhu.edu> writes:
On Sun, 03 Apr 2011 09:49:44 -0400, Andrei Alexandrescu  
<SeeWebsiteForEmail erdani.org> wrote:


 On 4/3/11 6:38 AM, Jacob Carlborg wrote:

 On 2011-04-02 23:33, Jonathan M Davis wrote:

 On 2011-04-02 09:26, Jacob Carlborg wrote:

 On 2011-04-01 11:01, Jonathan M Davis wrote:

 On 2011-03-29 14:06, Jacob Carlborg wrote:

 I've made a few minor changes:

 * Renamed EmailStatusCode.Off -> None and On -> Any
 * Added and clarified the documentation for EmailStatusCode.Any and
 None
 * Updated the documentation

 Github: https://github.com/jacob-carlborg/phobos/tree/isemail
 Docs: http://dl.dropbox.com/u/18386187/isemail.html


 Shouldn't isEmail have a template constraint which verifies that the
 e-mail variable is a valid string type? Also, it would probably be
 better to use Char instead of T for the type. It's clearer that way.  
 I
 believe that that's what std.string does.


 That's a good idea.


 I'd say that pretty much every template should have a template
 constraint on
 it. I'm not sure that I've ever seen a template which was flexible
 enough that
 it would compile with anything you gave it, and if there's an argument
 that
 you can give it which won't compile with it, it should fail the  
 template
 constraints, otherwise the error messags are much harder to understand
 and
 track down.


 I've added a static assert with a custom error message. I think static
 asserts can give better error messages than template constraints.


 Please use a template constraint. This is the right thing to do and we  
 use it throughout Phobos.

 Thanks,

 Andrei


Also, static assert doesn't compose with is(typeof(...)) and  
__traits(compiles,...). (It stops the compiler if it's every evaluated)  
I'd recommend simply putting a string on an empty line over static assert:

static assert(...,"My error message");

static if(...) "My error message";
Apr 03 2011
prev sibling parent Jacob Carlborg <doob me.com> writes:
On 2011-04-03 15:49, Andrei Alexandrescu wrote:

 On 4/3/11 6:38 AM, Jacob Carlborg wrote:

 On 2011-04-02 23:33, Jonathan M Davis wrote:

 On 2011-04-02 09:26, Jacob Carlborg wrote:

 On 2011-04-01 11:01, Jonathan M Davis wrote:

 On 2011-03-29 14:06, Jacob Carlborg wrote:

 I've made a few minor changes:

 * Renamed EmailStatusCode.Off -> None and On -> Any
 * Added and clarified the documentation for EmailStatusCode.Any and
 None
 * Updated the documentation

 Github: https://github.com/jacob-carlborg/phobos/tree/isemail
 Docs: http://dl.dropbox.com/u/18386187/isemail.html


 Shouldn't isEmail have a template constraint which verifies that the
 e-mail variable is a valid string type? Also, it would probably be
 better to use Char instead of T for the type. It's clearer that way. I
 believe that that's what std.string does.


 That's a good idea.


 I'd say that pretty much every template should have a template
 constraint on
 it. I'm not sure that I've ever seen a template which was flexible
 enough that
 it would compile with anything you gave it, and if there's an argument
 that
 you can give it which won't compile with it, it should fail the template
 constraints, otherwise the error messags are much harder to understand
 and
 track down.


 I've added a static assert with a custom error message. I think static
 asserts can give better error messages than template constraints.


 Please use a template constraint. This is the right thing to do and we
 use it throughout Phobos.

 Thanks,

 Andrei


Ok, if you say so but the error message won't be as good as with a 
static assert.

-- 
/Jacob Carlborg
Apr 03 2011
prev sibling next sibling parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 3/29/11 4:06 PM, Jacob Carlborg wrote:

 I've made a few minor changes:

 * Renamed EmailStatusCode.Off -> None and On -> Any
 * Added and clarified the documentation for EmailStatusCode.Any and None
 * Updated the documentation

 Github: https://github.com/jacob-carlborg/phobos/tree/isemail
 Docs: http://dl.dropbox.com/u/18386187/isemail.html


Overall I think the module is in good shape for admission, with the nits 
below. Here is my review:

* First doc sentence: "To validate..." is grammatically mistaken 
(doesn't have a predicate). Normally I'd just note that and fix later, 
but this is the first sentence so I guess it gets a higher priority.

* Copyright: please look up and use the comment convention used 
elsewhere in Phobos.

* Instead of using a bool for checkDns I suggest enum CheckDns { no, yes 
} which is used in other parts of Phobos.

* Line 67: Please use template constraints instead of static assert.

* Line 79 and others: I wish there were no empty line before the "else" 
clause...

* Line 86: There is a constant Threshold that doesn't follow common 
conventions.

* Line 189: enforce?

* Line 235: At best 33, 126, and 10 were symbolic constants or otherwise 
explained.

* Line 920: EmailStatus should be either private or documented.

* Line 1316: Why is Token a struct with an enum in it instead of an 
enum? (Also the static: label is unnecessary.)

* Line 1357: Why is there a need for conversion to int? Wouldn't dchar 
work well?


Andrei
Apr 03 2011
parent reply Jacob Carlborg <doob me.com> writes:
On 2011-04-03 20:09, Andrei Alexandrescu wrote:

 On 3/29/11 4:06 PM, Jacob Carlborg wrote:

 I've made a few minor changes:

 * Renamed EmailStatusCode.Off -> None and On -> Any
 * Added and clarified the documentation for EmailStatusCode.Any and None
 * Updated the documentation

 Github: https://github.com/jacob-carlborg/phobos/tree/isemail
 Docs: http://dl.dropbox.com/u/18386187/isemail.html


 Overall I think the module is in good shape for admission, with the nits
 below. Here is my review:

 * First doc sentence: "To validate..." is grammatically mistaken
 (doesn't have a predicate). Normally I'd just note that and fix later,
 but this is the first sentence so I guess it gets a higher priority.


I'll fix that.


 * Copyright: please look up and use the comment convention used
 elsewhere in Phobos.


Ok.


 * Instead of using a bool for checkDns I suggest enum CheckDns { no, yes
 } which is used in other parts of Phobos.


Why is this better? This is just like the pointless aliases you want to 
avoid. Why do we even have a bool type in the language if we're going to 
replace it with enums all the time?


 * Line 67: Please use template constraints instead of static assert.


Ok, but the error message won't be as good as with the static assert.


 * Line 79 and others: I wish there were no empty line before the "else"
 clause...


I'm not changing that, if you want to do it fine, go ahead.


 * Line 86: There is a constant Threshold that doesn't follow common
 conventions.


Forgot that one.


 * Line 189: enforce?


Sure.


 * Line 235: At best 33, 126, and 10 were symbolic constants or otherwise
 explained.


I'll do something about this.


 * Line 920: EmailStatus should be either private or documented.


EmailStatus is documented, note the three slashes, which is a doc comment.


 * Line 1316: Why is Token a struct with an enum in it instead of an
 enum? (Also the static: label is unnecessary.)


Hmm, I think I thought that if Token was an enum it wouldn't be 
implicitly convertable to string but it seems I was wrong.


 * Line 1357: Why is there a need for conversion to int? Wouldn't dchar
 work well?


I just did a straight port of the PHP code and that's how it looked. 
I'll see if I can do something about it.


 Andrei


-- 
/Jacob Carlborg
Apr 03 2011
parent reply Jonathan M Davis <jmdavisProg gmx.com> writes:
On 2011-04-03 13:13, Jacob Carlborg wrote:

 On 2011-04-03 20:09, Andrei Alexandrescu wrote:

 On 3/29/11 4:06 PM, Jacob Carlborg wrote:

 I've made a few minor changes:
 
 * Renamed EmailStatusCode.Off -> None and On -> Any
 * Added and clarified the documentation for EmailStatusCode.Any and None
 * Updated the documentation
 
 Github: https://github.com/jacob-carlborg/phobos/tree/isemail
 Docs: http://dl.dropbox.com/u/18386187/isemail.html


 
 Overall I think the module is in good shape for admission, with the nits
 below. Here is my review:
 
 * First doc sentence: "To validate..." is grammatically mistaken
 (doesn't have a predicate). Normally I'd just note that and fix later,
 but this is the first sentence so I guess it gets a higher priority.


 
 I'll fix that.
 

 * Copyright: please look up and use the comment convention used
 elsewhere in Phobos.


 
 Ok.
 

 * Instead of using a bool for checkDns I suggest enum CheckDns { no, yes
 } which is used in other parts of Phobos.


 
 Why is this better? This is just like the pointless aliases you want to
 avoid. Why do we even have a bool type in the language if we're going to
 replace it with enums all the time?


Andrei has been pushing for this, because it makes the calls to the function 
easier to read. If it's CheckDns.yes, then it's clear what you wanted, whereas 
if it were true, you'd have to look up whether true meant that it's supposed 
to check or skip checking. In some cases, that's more useful than others, but 
it's already becoming fairly common in Phobos that such explicit enums are 
used rather than bools.

- Jonathan M Davis
Apr 03 2011
parent Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 4/3/11 4:38 PM, Jonathan M Davis wrote:

 On 2011-04-03 13:13, Jacob Carlborg wrote:

 Why is this better? This is just like the pointless aliases you want to
 avoid. Why do we even have a bool type in the language if we're going to
 replace it with enums all the time?


 Andrei has been pushing for this, because it makes the calls to the function
 easier to read. If it's CheckDns.yes, then it's clear what you wanted, whereas
 if it were true, you'd have to look up whether true meant that it's supposed
 to check or skip checking. In some cases, that's more useful than others, but
 it's already becoming fairly common in Phobos that such explicit enums are
 used rather than bools.


This becomes painfully obvious in calls with more than one 
Boolean-encoded categorical argument.

auto r = getWidget("widget", true, false);

The topic is discussed in the book Code Complete and I suppose in many 
other style guides.


Andrei
Apr 03 2011
prev sibling parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 3/29/11 4:06 PM, Jacob Carlborg wrote:

 I've made a few minor changes:

 * Renamed EmailStatusCode.Off -> None and On -> Any


I suggest we follow the nascent convention that enum values start with a 
lowercase letter.

Andrei
Apr 03 2011
parent Jacob Carlborg <doob me.com> writes:
On 2011-04-03 20:22, Andrei Alexandrescu wrote:

 On 3/29/11 4:06 PM, Jacob Carlborg wrote:

 I've made a few minor changes:

 * Renamed EmailStatusCode.Off -> None and On -> Any


 I suggest we follow the nascent convention that enum values start with a
 lowercase letter.

 Andrei


Already fixed.

-- 
/Jacob Carlborg
Apr 03 2011