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

Make std.file.deleteme package. #5400

Closed
wants to merge 1 commit into from
Closed

Conversation

jmdavis
Copy link
Member

@jmdavis jmdavis commented May 18, 2017

There is no reason to expose a function that's purely for unit testing
Phobos.

@jmdavis jmdavis added the Trivial typos, formatting, comments label May 18, 2017
JackStouffer
JackStouffer previously approved these changes May 18, 2017
@joakim-noah
Copy link
Contributor

Agree, looking below, you can stick this function, the next struct and the system_directory stuff, which is only used in the tests also, in a larger version(unittest) block.

There is no reason to expose a function that's purely for unit testing
Phobos.
@jmdavis
Copy link
Member Author

jmdavis commented May 18, 2017

Okay. I updated it as suggested and put all 3 of those sections in the same version(unittest) block.

@CyberShadow
Copy link
Member

Careful, deleteme is used throughout all examples. Making it inaccessible will break all runnable examples that use it.

If it's useful enough for examples, it might be useful for real code as well. Can it be promoted to a "real" public function instead?

@jmdavis
Copy link
Member Author

jmdavis commented May 19, 2017

Hmmm. This definitely seems like a downside to insisting on the examples being runnable as-is by anyone rather than just be understandable and informative. The fact that deleteme is even part of the examples isn't particularly great IMHO. It's just that for testing, the odds of accidentally reusing an existing file or directory go down by using deleteme, so it makes some sense to use it in the tests, and that bled into the examples, since the examples are tests. I suppose that it could be argued that since it's useful for std.file's unit tests, it's useful for 3rd party unit tests as well, but even so, IMHO, it's kind of an ugly name for a public API. If we want to truly expose it, we should probably give it a better name. Maybe testPath? I don't know.

@joakim-noah
Copy link
Contributor

Is there some way it can be removed from the runnable examples? I always felt it distracted from them, by adding file management baggage. @wilzbach, what do you think?

@schveiguy
Copy link
Member

I'd vote for actually making it more public, and making the functionality more robust.

@JackStouffer JackStouffer removed the Trivial typos, formatting, comments label May 26, 2017
@joakim-noah
Copy link
Contributor

Pinging @wilzbach again, regardless of whether deleteme is made more public, what do you think of removing it from the documented and runnable examples, perhaps by adding some sed/regex script that's run on the examples? It adds another thing for noobs to figure out, which is really an artifact of Phobos always trying to clean up temporary files.

btw, I just tried the same runnable example that was crashing for me before and wasn't able to reproduce anymore. :)

@wilzbach
Copy link
Member

Is there some way it can be removed from the runnable examples? I always felt it distracted from them, by adding file management baggage. @wilzbach, what do you think?

Hmm from what I understand, there are at least these three options to go:

  • use the existing assert -> writeln pipeline to translate deleteme() in public unittests to sth. like test (probably wouldn't be difficult)
  • rewrite the public examples in code to use a fixed filename
  • expose deleteme as sth. like tempFile

I honestly have to admit that I would prefer having something like tempFile that maybe returns a std.stdio.File and delete itself once its destructed. I know that there is std.stdio.File.tmpfile, but it returns a file without a name.
There was even a recent discussion about tempFile on the NG (tempFile has it's own problems).

Pinging @wilzbach again, regardless of whether deleteme is made more public, what do you think of removing it from the documented and runnable examples, perhaps by adding some sed/regex script that's run on the examples? It adds another thing for noobs to figure out, which is really an artifact of Phobos always trying to clean up temporary files.

Hmm imho with a proper name, there wouldn't be any need for a noob to figure out what it means.
However, yes it would be rather trivial to such a replacement, but don't forget this also adds another thing to our pipeline - i.e. how is a contributor supposed to know that all deletemes get auto-magically replaced?
@CyberShadow got rightfully conservative about making the build setup more complex.

btw, I just tried the same runnable example that was crashing for me before and wasn't able to reproduce anymore. :)

Great :)

@joakim-noah
Copy link
Contributor

I agree that simply renaming deleteme is the best approach.

@jmdavis, can you amend this PR to make deleteme public and give it a new name that you think is best, along with a little ddoc on what it's for? Once you do that, we can get this in.

@MetaLang
Copy link
Member

@jmdavis do you still intend to continue with this? If not can you please close it?

@jmdavis jmdavis closed this Nov 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants