-
-
Notifications
You must be signed in to change notification settings - Fork 741
localize some imports #2395
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
localize some imports #2395
Conversation
calling $(D to!string)) when retention is needed. If the caller needs | ||
to retain a copy of every line, use the $(LREF byLineCopy) function | ||
calling $(D to!string)) when retention is needed. If the caller needs | ||
to retain a copy of every line, use the $(LREF byLineCopy) function | ||
instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm? What got changed here? Whitespace? I can't see any difference in this diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see any difference in this diff.
Line ending?
Overall, LGTM. I think this is an important direction to take; it should get us closer to the point where monsters like |
LGTM |
Auto-merge toggled on |
localize some imports
I vote to call this process scoping imports rather than localizing them, due to the word localize typically being used in the context of internationalization and translation of strings. (I was confused at first how you could localize an import when I saw the merge notification on the IRC) |
@@ -742,6 +742,7 @@ string toHexString(Order order = Order.increasing, LetterCase letterCase = Lette | |||
result[i++] = hexDigits[u & 15]; | |||
} | |||
} | |||
import std.exception : assumeUnique; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one will obviously be different, as it's already merged, but aren't imports usually declared at the start of the inner-most scope where they're used, rather than right before they're used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no strict policies about it I am aware of. I personally would favor such style (beginning of scope) too but it doesn't seem critical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember @9rnsr once asking for imports to go at the top of a local scope, as failure to do so could introduce subtle name resolution bugs. I can't remember how though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The explanation is here: #1813. Though I don't completely buy it though, I guess one could argue it's good style anyways?
Ditto. |
We need tooling for this, it's too error prone to do by hand. |
No description provided.