Skip to content

Conversation

@JackStouffer
Copy link
Contributor

@JackStouffer JackStouffer commented Jan 24, 2018

Using the toString format from #5991, update Date's string methods to accept output ranges.

@dlang-bot
Copy link
Contributor

dlang-bot commented Jan 24, 2018

Thanks for your pull request, @JackStouffer!

Bugzilla references

Auto-close Bugzilla Description
10828 datetime toString functions should accept sink

Copy link
Contributor

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

+/
string toISOExtString() const @safe pure nothrow
/// ditto
void toISOString(W)(ref W writer) const if (isOutputRange!(W, char))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: constraints should be indented on the same level as the function declaration.

}

/// ditto
void toSimpleString(W)(ref W writer) const if (isOutputRange!(W, char))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: constraints should be indented on the same level as the function declaration.

}

/// ditto
void toString(W)(ref W writer) const if (isOutputRange!(W, char))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: constraints should be indented on the same level as the function declaration.

import core.time;// : TimeException;
import std.traits : isSomeString, Unqual;
import std.typecons : Flag;
import std.range.primitives;// : isOutputRange;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use selective imports for new imports - otherwise you risk leaking the entire module.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it was the opposite?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't convert the existing imports to selective imports because that would close the symbol leakage: #5584

@JackStouffer
Copy link
Contributor Author

These random vibed errors are getting pretty annoying

@wilzbach
Copy link
Contributor

Yeah, I am opening a new Jenkins issue almost every second day - https://github.com/dlang/ci/issues (not mentioning hitting the existing ones), but I don't have a concrete proposal except for dropping Jenkins, creating one common bash script and running that one all repos on a reliable CI (e.g. CircleCi or SemaphoreCI). As we already have the build scripts for Jenkins it probably wouldn't take that long to do...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants