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
next sibling 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
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

EmailStatusCode.any

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
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

EmailStatusCode.any

that's the case I'll change the names to begin with lowercase.


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).

> 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 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

EmailStatusCode.any

that's the case I'll change the names to begin with lowercase.


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).

> 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

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

EmailStatusCode.any

that's the case I'll change the names to begin with lowercase.


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).

 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

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
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 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

EmailStatusCode.any

that's the case I'll change the names to begin with lowercase.


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).

 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

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
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 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

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. =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).

That part is clear. =20
 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. =20 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. =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=





 Renaming E to e, on the other hand, that's a lot worse.
=20
 -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. =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=




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




 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. =20 -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. =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.

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 next sibling 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 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.


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 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:

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 next sibling parent 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:

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
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
next sibling parent 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
prev sibling 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:

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 next sibling parent 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
prev sibling 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 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

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. =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).

That part is clear. =20
 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. =20 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. =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=





 Renaming E to e, on the other hand, that's a lot worse.
=20
 -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. =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=




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




 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. =20 -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. =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.

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 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-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
prev sibling next sibling parent "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

EmailStatusCode.any

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
prev sibling next sibling 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

EmailStatusCode.any

that's the case I'll change the names to begin with lowercase.


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).

> 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 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

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. =20
 All type names begin with an uppercase letter, and all variables
 begin with a lowercase letter (with the possible exception of priva=






 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. =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=





 been discussed.

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




 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=




 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=




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




 e, on the other hand, that's a lot worse.
=20
 -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. =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=



 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/templat=


 that evaluate to values) are camelCased.
=20
 -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. =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
prev sibling parent "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
prev sibling next 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 next sibling parent 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
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 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
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 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
prev sibling 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 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
next sibling 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 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 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
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