-
-
Notifications
You must be signed in to change notification settings - Fork 701
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
fix Issue 12368 - std.file.write conflicts with std.stdio.write #2011
Conversation
Yeah, I think that's by design... |
Yah, it doesn't seem this is needed. At some point or another, libraries (including stdlib) and applications will expose the same name from different modules. It doesn't seem that adding aliases like this would scale well. |
@andralex Shouldn't we try and prevent these conflicts at least within phobos? The I love that the module system lets us disambiguate manually, but it would be better not to have to... |
But this changes "call std.file.write" with "call fileWrite". Hardly progress. Also instead of using well-known qualification people need to learn a new symbol. How is this any better? |
I guess, although the old name is retained, so it's an alternative and not a replacement. I'm more concerned about future phobos additions introducing more conflicts. |
It's more churn for no progress.
I understand. Let's cross that bridge when we get to it. OK to close? |
There have been lengthy discussions about unambiguous contextual names versus pushing the burden of disambiguating on the user. Although it's a bit of a hand-wavy argument, I think the consensus generally gravitates towards the latter approach, and that is certainly the approach exercised in Phobos today. If someone wants to change this - as I said in the bug report too - it should be taken up in dm.D, not in a small Github PR. |
After posting this, I realized that this was not, in fact, the change that I wanted. The problem with disambiguating conflicts using the fully-qualified module name is that it doesn't work with UFCS chaining. However, the Anyway, I'd like to reply to two arguments posted here:
I sort of disagree here, because discussions on the NG come and go, lots of things and proposals and arguments are presented and ultimately no one comes up with something concrete and the discussion stalls and nothing gets done. A GitHub PR presents a concrete change that can be reviewed on its own merits. Second, Jakob brought up the renamed import argument. I'm not sure I like the idea of renamed imports in general, because these are only a little better than pasting a bunch of custom helper functions at the top of each module. In effect, you are creating your very own tiny adapter library, with new symbols that map to symbols in the standard library, which everyone reading your code needs to know. They won't be encapsulated properly and they will likley be different from one programmer to another. These module-local "customizations" bother me more than the other proposed solutions, personally.
Do we want a |
I think there's merit in |
How about we make that change, document it nicely and with examples, all in this pull request. |
Agreed, |
Hmm. I've been thinking. A
So, there would be two overloads of |
YES. I was thinking the exact same thing. |
All right, pushed first |
Oh, I was expecting |
I added a convenience shorthand which takes a filename too, if that's OK :) |
recurrence!((a, n) => x + y*1i + a[n-1]^^2)(0+0i) | ||
.take(ubyte.max) | ||
.countUntil!(z => z.re^^2 + z.im^^2 > 4)) | ||
) |
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.
since this is on its own line the previous one should be as well
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.
Like this?
iota(-1, 3, 2.0/size).map!
(
y => iota(-1.5, 0.5, 2.0/size).map!
(
x => cast(ubyte)(1+
recurrence!((a, n) => x + y*1i + a[n-1]^^2)(0+0i)
.take(ubyte.max)
.countUntil!(z => z.re^^2 + z.im^^2 > 4))
)
).toFile(stdout);
It looks a bit weird...
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.
no, the previous closing paren
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.
Oh, you mean the next one (since this comment is attached to the first one's line). Okay.
lgtm modulo nits |
I'd say no shorthand; for consistency we'd need to put it in std.file, which is odd. |
|
That'll open it for reading, not writing... and you'll almost surely want "b" so your binary data doesn't get corrupted. |
So, what of the shorthand? Aye or nay? It appears to be hanging by the |
r.front.toFile(f); | ||
r.popFront(); | ||
} | ||
} |
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, why is this function better than:
r.copy(f);
?
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.
File
does not have a range interface,LockingTextWriter
doesLockingTextWriter
does some things with strings to put them in theFILE*
's orientation- Since we know it doesn't make sense to write pointers to files, we can have
toFile
write arrays and ranges of any number of dimensions (see the example).
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.
File
is not an output range. You have to use File.lockingTextWriter
for that, which apparently deals with text.
edit:
Oops, didn't refresh.
This pull request hung on all the non-windows testers |
|
OK, fixed up and updated. |
Proper/safe file writing is beyond me, so I can't really comment on the quality, but I am glad this isn't dead as it has some cool changes :) |
|
.countUntil!(z => z.re^^2 + z.im^^2 > 4)) | ||
) | ||
) | ||
.copy(stdout.lockingBinaryWriter); |
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.
👍 Love this code example.
FILE* fps_; | ||
|
||
// the unshared version of fps | ||
@property _iobuf* handle_() @trusted { return cast(_iobuf*)fps_; } |
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.
space after )
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.
Auto-merge toggled on |
Auto-merge toggled off |
I approve the addition with the nits mentioned. Go for it! |
|
Fixed style nits. |
|
|
Auto-merge toggled on |
https://d.puremagic.com/issues/show_bug.cgi?id=12368