Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

std.net.isemail #27

Closed
wants to merge 36 commits into from
Closed

std.net.isemail #27

wants to merge 36 commits into from

Conversation

jacob-carlborg
Copy link
Contributor

No description provided.

* Removed pointless aliases
* Change the const members in EmailStatus to properties
* Clarified the version in the module doc
* Made the EmailStatus' constructor private
* Formatting of the copyright
* Changed the checkDNS bool to an enum
* Changed the isEmail static assert to a template constraint
* Changed to the correct naming convention for all the enum members
* Make use of "enforce" where appropriate
@andralex
Copy link
Member

andralex commented Apr 9, 2011

I wanted to merge this and there really seem to be no limit on the line length. I mean there are lines 195 columns long. Could we please reach some consensus? Anything toward 200 columns in a line are unreasonable by the most lax standards. Please.

@jacob-carlborg
Copy link
Contributor Author

You're absolutely right. I'll cut it down to 120 columns, will that be ok?

@jacob-carlborg
Copy link
Contributor Author

I've reformatted the code to fit within 120 columns.

@andralex
Copy link
Member

enum CheckDns { yes, no } must be enum CheckDns { no, yes } such that people who test a CheckDns value with if (v) or if (!v) get the expected result.

@jacob-carlborg
Copy link
Contributor Author

Done.

@jmdavis
Copy link
Member

jmdavis commented Apr 20, 2011

This has been merged into the main repository now, hasn't it? Or am I missing something? If so, this pull requests should be closed.

@jacob-carlborg
Copy link
Contributor Author

As far as I can see it has. I didn't noticed it's been merged until I read in the Phobos mailing list that it didn't compile on 64bit. BTW, whose responsibility is it to close a pull request, the one making the request or the one merging it?

@jmdavis
Copy link
Member

jmdavis commented Apr 20, 2011

Normally, a pull request is closed automatically when it's merged in. The fact that this one didn't close indicates that whoever checked in the code didn't do it via a merge from jacob-carlborg:isemail. So, I don't know what happened.

In any case, a pull request should be closed when the changes are merged in, and normally it happens automatically. If for some reason, it doesn't, then I'd say that the person who merged in the code needs to close it manually, but it normally happens automatically.

@jmdavis jmdavis closed this Apr 20, 2011
@andralex
Copy link
Member

That must be me. I'm not sure what the heck happened. I'll close the pull requiest now.

@andralex
Copy link
Member

Oh wait someone closed it already :o).

@jmdavis
Copy link
Member

jmdavis commented Apr 23, 2011

I would point out though that the line lengths haven't been fixed. Some of them are well beyond 120 characters, so they definitely don't follow the 80/120 rule discussed on the Phobos list, and at some point that should be fixed.

@jacob-carlborg
Copy link
Contributor Author

I didn't bother with some of the comments regarding bugs. When the bugs are fixed then comments will be removed, problem solved. If you're referring to the few lines where the length exceeds the limit with a few characters it's just annoying to have to break a line that is just one or two characters too long.

@jmdavis
Copy link
Member

jmdavis commented Apr 23, 2011

I was referring to the fact that some of the lines exceed 120 characters. The discussion on line length on the Phobos list resulted in the decision that we'd have a soft limit of 80 characters and a hard limit of 120. So, in general, lines should be within 80 characters, but they can go over if they need to. However, they can never exceed 120 characters. Quite a few lines in std.net.isemail exceed 120 characters. I know that the module was written before that decision was made (and it was actually the reason that Andrei pushed for a character line limit), so unless you had specifically gone and taken the time to fix the overly long lines (which you evidently haven't), then they obviously aren't going to have been fixed. I'm just saying that at some point they're going to need to be fixed.

The 120 character limit (rather than the 80 that Andrei was originally pushing for) makes it so that most of the lines are okay (I'd hate to have to make that module fit within 80 character lines as much as you would), but the 120 character limit is a hard limit, so it can't be exceeded even by a single character, and a number of lines need to be shortened. I'm not saying that it's something that needs to be done right away, but it's going to need to be done at some point, and unless you do it, someone else likely will eventually.

@jacob-carlborg
Copy link
Contributor Author

Ok. I've cut down the column limit to 120 where it previously exceeded the limit. Should the pull request be reopened?

@jmdavis
Copy link
Member

jmdavis commented Apr 24, 2011

No, do a fresh pull request. And since it appears that Andreid didn't merge it in a manner that made the pull request close on its own, there's likely to be merge conflicts if you haven't done the changes on what's currently checked in in the main repository, though I'm only aware of one change to std.net.isemail since it was added to Phobos (the fix for the 64-bit compilation error).

@jacob-carlborg
Copy link
Contributor Author

Ok, I'll make a new pull request.

On 24 apr 2011, at 14:37, jmdavisreply@reply.github.com wrote:

No, do a fresh pull request. And since it appears that Andreid didn't merge it in a manner that made the pull request close on its own, there's likely to be merge conflicts if you haven't done the changes on what's currently checked in in the main repository, though I'm only aware of one change to std.net.isemail since it was added to Phobos (the fix for the 64-bit compilation error).

Reply to this email directly or view it on GitHub:
#27 (comment)

NilsBossung pushed a commit to NilsBossung/phobos that referenced this pull request Jun 15, 2013
JohanEngelen pushed a commit to weka/phobos that referenced this pull request Mar 25, 2016
std.math: Do not use inline assembly for expi().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants