std.socket clean-up

[CyberShadow]

Follow-up to pull request #180.

[CyberShadow]

By the way, I'd appreciate if someone could test this on a Posix system.

[CyberShadow]

I decided to dig through Bugzilla's search results for "socket". Aside from some small fixes, I've found an update to std.socket by Christopher Miller, the module's original author, which I've committed with his name and the patch's post date, and brought up to date to the current tree. Commits incoming:

[CyberShadow]

Given the lack of comments and seeing how I keep finding things to improve in std.socket, I guess it'd be better to wait until I finish messing with it and get a community review going or something.

[JakobOvrum]

Strange lack of comments here.

I know I'm not alone in waiting eagerly for this module to become properly usable. I'm glad you've picked it up, and whether it'll be in the next release or not, keep up the good work!

[jmdavis]

If you're making a lot of changes to std.socket - especially if they involve many API changes, then it would be better to have a community review. Particularly, if you're going to do a bunch of changes on top of this and your changes affect the API, then we should probably think about having a formal review for the changes. And if std.socket really needs some design changes, then I definitely think that it would be better to take the time to do them and have a proper review rather than doing a lot of bandaid fixes. But if the std.socket just needs a lot of smaller fixes, then a formal review is overkill. So, the question is how much redesign the module needs how large the changes are going to be.

On a related note, at some point soon, the enums value names are going to need to be fixed to be properly camelcased per the Phobos naming conventions (which unfortunately, is a breaking change which can't be done via the normal deprecation path, since there's no way to deprecate enum values without breaking code that uses final switch or std.traits.enumMembers). And from the little I've looked at std.socket, I get the impression that the enums overlap way too much, which at least implies that a module redesign should be done, but I've never used the module, so I don't know. In any case, if we're going to do a redesign, the enum values should be fixed then rather than now, but if we definitely aren't going to do a module redesign, then we should make those changes soon.

So, in any case, deciding on how to go about all of this depends on the question of whether std.socket needs a redesign or just minor fixes.

[CyberShadow]

So far, I've tried to minimize breaking changes to the module. I'm not sure if Chris Miller's patch breaks anything, though.

The next thing on my list is to review how much does std.socket depend on IPv4, and how much work would be required to make it protocol-agnostic (using the new getnameinfo/getaddrinfo APIs). I'll probably decide whether it needs a rewrite at that point, though it probably won't be hard to preserve backwards-compatibility. I might also make a newsgroup post to collect some complaints and opinions regarding the module.

Very little should ever be deprecated immediately, because doing so breaks code. Unless errorCode was very recently added, it needs to be scheduled for deprecation before it's deprecated.

What's the proper way to soft-deprecate something?

[jmdavis]

std.string and std.file have some examples of stuff which is scheduled for deprecation. If the symbol in question isn't templated, then all you need to do is put something similar to this at the top of its ddoc comment:

   $(RED Scheduled for deprecation in January 2012.
      Please use $(XREF ascii, hexDigits) instead.)

Normally, the date should be 6 months from the next release, but you can just skip the date, and I'll fix it to whatever is appropriate once it's been merged in. Regardless, it needs something in the ddoc comment indicating that it's scheduled for deprecation and telling you what to use instead of there is something which is replacing it.

If the symbol is templated, then it should have a pragma message in its body informing the programmer that its scheduled for deprecation (similar to the message in the comment). The message then gets printed if you compile code that uses it. And again, std.string and std.file have examples of that. However, that doesn't really apply in this case given that errorCode isn't templated.

[jmdavis]

Okay. So, you need to do some research into whether std.socket really needs a redesign or not and how much of one it needs if it does need one.

The question then is whether you feel that this pull request should still be reviewed and merged in if it's solid, or should it wait for the redesign (or until it's decided that one isn't needed)?

[CyberShadow]

I'll gladly accept feedback for the commits so far (and I don't think it would be obsoleted by the redesign), but I don't think it should be merged right now. With so many changes, I'd need to find someone with a good body of D2 network code to test it, or test it myself while porting my D1 network framework and applications to D2 (which is what started me working on std.socket in the first place).

[jmdavis]

If we're not looking to merge these changes, then there's no reason to keep this pull request open. I suggest that you bring it up in the newsgroup if you want feedback. You're more likely to get it that way. Once you have something that's ready to be merged in, then either create a pull request then, or get a formal review of it in the newsgroup (which will be necessary if the changes are extensive - particularly if they make major changes to the API).

[CyberShadow]

I didn't think anyone would mind an open pull request. People are reminded to review open pull requests on the newsgroup, but I haven't gotten to the point where I'm done making changes and ask for feedback. (I guess I could make a posting somewhere, but make it clear that right now I'm only accepting feedback on my changes, and not suggestions for the module as a whole.)

[jmdavis]

The problem is that there really aren't all that many people who actively look at Phobos pull requests (or if there are more, they don't comment). There are a few Phobos developers who actively look them over and some folks beyond that who look them over fairly frequently, but you're just not going to get a lot of feedback with a pull request sitting here on github. And pull requests that have many comments tend to not get commented on by more people all that often, so just leaving it open isn't likely to attract many new reviewers.

In addition, because there are just a few Phobos devs who are very active about going over pull requests, one more open pull request is one more pull request that we have to look over and determine whether there's anything that we need to do with it right now. So, leaving it open when it isn't ever going to be merged gets in the way.

You're just more likely to get feedback on the newsgroup, and it's likely to be far more constructive. And larger changes generally need to be discussed there (and probably formally reviewed there) anyway.

[CyberShadow]

OK, thanks.

[CyberShadow]

Ugh, new commits don't show up in closed pull requests. Having a list of relevant commits along with comments on one page was quite convenient (merges mess up the branch history).

It doesn't matter much at this point, but I've put a [WIP] tag in the title as a possible compromise.

[CyberShadow]

Never mind, I found how to get a list of only my commits on a branch:

CyberShadow/phobos@master...new-std-socket

[CyberShadow]

I've addressed all issues brought up on the newsgroup, so I think it's time to reopen this pull request. Or should I post a new one?

[jmdavis]

A new one. That way, the comments in this one don't get in the way, and people seem to be more likely to look at fresh pull requests than old ones.