Document what happens on failure in path ext is_file is_dir #42926

Havvy · 2017-06-27T11:04:28Z

Also, what other ways could there be an error that gets discarded and returns false? Should we list them all? Should we say that any errors trying to access the metadata at that path causes it to return false, even if there might be a file or directory there?

Should I add a See also link to the original functions that do return Results?

alexbowers · 2017-06-27T11:06:29Z

This stemmed from a discussion in #rust-beginners today, and tripped me up for a while. Support better documenting it.

steveklabnik · 2017-06-27T20:27:09Z

Also, what other ways could there be an error that gets discarded and returns false? Should we list them all?

I'm not sure, but I also don't mind incrementalism.

Should I add a See also link to the original functions that do return Results?

👍

steveklabnik · 2017-07-05T14:43:31Z

@Havvy ping on the "See also"

Havvy · 2017-07-05T22:04:19Z

Log from #rust-docs (with permission from @projektir and @GuillaumeGomez (a.k.a. imperio)

<Havvy> "If you want to check for errors, call fs::metadata and check the Result for that for errors. Then call fs::Metadata::is_file on that."
<Havvy> Can I get some editorial opinion on this?
<imperio> Havvy: seems like super heavy O.o
<imperio> (super heavy way to say it to be more clear)
<Havvy> (For context, this is for Path::is_file and variants)
<projektir> Result seems like a fairly established pattern in Rust, idk if it's worth pointing out too much.
<projektir> calling "metadata" to get errors is the weird part here
<Havvy> So s/and check the result for//
<projektir> so metadata will only error if a) you have no permissions or b) path doesn't exist
<projektir> but that seems like an odd way to check those things
<Havvy> AFAICT, Path::is_file is a convenience function to call when you don't care about errors, and if you do, you need to call fs::metadata, check the error, and then call fs::Metadata::is_file on that.
<imperio> projektir: it's doc so let's be as much explicit as possible
<Havvy> Hmm...
<projektir> imperio: most docs don't seem to mention this. They just return, well, "Result"
<projektir> I don't know if docs is the place to explain what Result/Option are
<projektir> api docs*
<imperio> projektir: no no, my point was, if you want to check error, let's explain how
<imperio> but otherwise, we don't care
<imperio> no and it shouldn't
<Havvy> Hmm... "This is a convenience function that ignores errors. If you want to check errors, call fs::metadata and handle its Result. Then call fs::Metadata::is_file if it was Ok."
<projektir> Havvy: for the record that reads very odd to me. Not the docs you're writing, but that the function is used that way
<projektir> it seems very, very odd to call something called "fs::metadata" to tell that your paths are messed up lol
<projektir> I would expect "is_valid_path" or w/e
<projektir> "can I get this metadata?" can be used in the process, but that's more for sanity
<Havvy> projektir: If it helps... Path::is_file is `fs::metadata(self).map(|m| m.is_file).unwrap_or(false)`
<projektir> yeah, I see how you got there
<projektir> but isn't the point of is_file() that you don't care if there are errors? :P
<imperio> Havvy: seems better
<Havvy> projektir: Yes, but that's only after you know there's fs::Metadata::is_file. It's not possible to know which function the reader will find first.
<projektir> seems like the situation is "is_file will also return false if calling fs:metadata resulted in errors"
<Havvy> Anecdotatly, at least one person has stumbled upon Path::is_file without knowing about fs::Metadata::is_file.
<projektir> but this should be wrapped by "don't use this if you care about which exact error it was"
<projektir> yeah current doc (on stable) doesn't seem to covert this well
<projektir> it just says false on symbolic links and doesn't really warn that is_file will eat your errors
<Havvy> projektir: Which is why I'm working on that: https://github.com/rust-lang/rust/pull/42926
<projektir> so steveklabnik seems in favor of including hte internal function (I am a bit iffy on this: what if it changes? generally, you document the behavior not the inner workings I would think)
<Havvy> projektir: Even if the internal function changes, it'll still have to be correct because otherwise you are breaking code.
<projektir> yeah but then you're referencing a function that's no longer in use
<projektir> "This will return false if the internal call to fs::metadata fails, such as in the case of broken symbolic links, invalid paths, or permission problems."
<projektir> In case of broken symbolic links this will return false.
<projektir> Oh ignore that last sentence
<Havvy> Also, can just update the docs at that point.
<projektir> then link to fs::metadata and they can see it returns Result
<projektir> Havvy: yes, this doesn't really matter I'm just being on the paranoid side (I'm used to docs being forgotten)
<Havvy> Are we okay with explicitly saying there's an internal call like that?
<projektir> Havvy: well, that's my interpretation of steveklabnik's approval of "
<projektir>     Should I add a See also link to the original functions that do return Results?
<projektir> "
<projektir> That's why I brought up the question of treating this as a black box api call vs getting into what's called internally
<projektir> because if I did the former I'd never mention fs::metadata at all, I'd just say what makes it error
<projektir> s/makes it error/makes it return false
<Havvy> projektir: I'm reading it differently, as the See Also section is a different section than the parts that say why it returns false.
<projektir> I guess I'm finding that kind of oddly coincidental
<projektir> if you're just saying "see also" to say "here's another useful function you can call if you want to know more about this path" that's dif

Havvy · 2017-07-05T22:07:00Z

From the log, there's a bit of contention on what we want to do exactly. Do we want to treat the implementation as a black box and list out errors that could occur, or do we want to specifically point out that the errors coerced to false are whatever fs::metadata fails on?

I'm of the opinion we want to treat it as a black box, and the current PR reflects that.

If we want to go the other way, I can word it as @projektir suggested towards the end of the IRC conversation.

projektir · 2017-07-05T22:08:01Z

src/libstd/path.rs

+    ///
+    /// # See Also
+    ///
+    /// This is a convenience function that ignores errors. If you want to check errors,


"ignores errors" doesn't seem quite right: the function will return false on errors, not ignore them.

I think API functions should be treated as black boxes and fs::metadata not so explicitly tied in here. It should just be mentioned in the "See also".

GuillaumeGomez · 2017-07-10T08:40:03Z

Travis failed:

[00:58:56] failures:
[00:58:56] 
[00:58:56] ---- path.rs - path::Path::is_dir (line 2278) stdout ----
[00:58:56] 	error: expected one of `!`, `.`, `::`, `;`, `?`, `{`, `}`, or an operator, found `Also`
[00:58:56]  --> <anon>:7:5
[00:58:56]   |
[00:58:56] 7 | See Also
[00:58:56]   |    -^^^^ unexpected token
[00:58:56]   |    |
[00:58:56]   |    expected one of 8 possible tokens here
[00:58:56] 
[00:58:56] error[E0425]: cannot find value `See` in this scope
[00:58:56]  --> <anon>:7:1
[00:58:56]   |
[00:58:56] 7 | See Also
[00:58:56]   | ^^^ not found in this scope
[00:58:56] 
[00:58:56] thread 'rustc' panicked at 'couldn't compile the test', /checkout/src/librustdoc/test.rs:273:12
[00:58:56] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:58:56] 
[00:58:56] 
[00:58:56] failures:
[00:58:56]     path.rs - path::Path::is_dir (line 2278)
[00:58:56] 
[00:58:56] test result: FAILED. 836 passed; 1 failed; 10 ignored; 0 measured; 0 filtered out

alexbowers · 2017-07-10T08:56:16Z

Interesting.

These are comments with /// # See Also is this a bug in the compiler?

GuillaumeGomez · 2017-07-10T12:21:22Z

No. "# See also" is invalid rust code. You want to use "//".

alexbowers · 2017-07-10T16:24:12Z

This is a comment though is it not?

Or are "triple comments" executed?

steveklabnik

i've got one tiny nit

steveklabnik · 2017-07-10T17:44:28Z

src/libstd/path.rs

    /// # Examples
    ///
    /// ```no_run
    /// use std::path::Path;
    /// assert_eq!(Path::new("does_not_exist.txt").exists(), false);
    /// ```
+    /// # See Also


could you put an extra /// above this please?

steveklabnik · 2017-07-10T17:45:57Z

src/libstd/path.rs

    /// # Examples
    ///
    /// ```no_run
    /// use std::path::Path;
    /// assert_eq!(Path::new("./is_a_directory/").is_dir(), true);
    /// assert_eq!(Path::new("a_file.txt").is_dir(), false);
+    /// # See Also


you need a triple backtick above this line, it's trying to execute

# See Also

as Rust code. the /// has nothing to do with it, really.

A /// below that backtick and above the /// # See Also line would be good as well.

Havvy · 2017-07-11T04:42:34Z

Oh my, I think this is actually mergable now.

GuillaumeGomez · 2017-07-11T08:01:08Z

Just one last thing: please squash your commits. Once done, r+.

alexbowers · 2017-07-11T15:22:10Z

@GuillaumeGomez Can't the merger squash? I thought that was supported by Github.

GuillaumeGomez · 2017-07-11T18:22:34Z

Only the bot can merge, so the one who opened the PR has to do it.

Havvy · 2017-07-12T18:04:48Z

Squashed

Mark-Simulacrum · 2017-07-12T18:05:50Z

@bors r=GuillaumeGomez

bors · 2017-07-12T18:05:50Z

📌 Commit 0b7e49c has been approved by GuillaumeGomez

Mark-Simulacrum · 2017-07-12T18:06:00Z

@bors rollup

steveklabnik · 2017-07-12T19:07:14Z

@bors: r+

bors · 2017-07-12T19:07:15Z

📌 Commit a01c91c has been approved by steveklabnik

steveklabnik · 2017-07-12T19:07:24Z

@bors: rollup- at @Havvy 's request

steveklabnik · 2017-07-12T19:08:33Z

@bors: rollup

I misunderstood @Havvy

@steveklabnik

Document what happens on failure in path ext is_file is_dir r? @steveklabnik Also, what other ways could there be an error that gets discarded and returns false? Should we list them all? Should we say that any errors trying to access the metadata at that path causes it to return false, even if there might be a file or directory there? Should I add a See also link to the original functions that do return Results?

Rollup of 7 pull requests - Successful merges: #42926, #43125, #43157, #43167, #43187, #43203, #43204 - Failed merges:

rust-highfive assigned steveklabnik Jun 27, 2017

arielb1 added the S-waiting-on-review label Jun 27, 2017

Mark-Simulacrum added S-waiting-on-author and removed S-waiting-on-review labels Jul 2, 2017

projektir reviewed Jul 5, 2017

View reviewed changes

steveklabnik suggested changes Jul 10, 2017

View reviewed changes

Havvy force-pushed the doc-path-ext branch from 80bf1f6 to 0b7e49c Compare July 12, 2017 18:02

Document error coercion to false in path-ext methods + see also sections

Loading
Loading status checks…

a01c91c

Havvy force-pushed the doc-path-ext branch from 0b7e49c to a01c91c Compare July 12, 2017 19:05

steveklabnik approved these changes Jul 12, 2017

View reviewed changes

steveklabnik mentioned this pull request Jul 13, 2017

Rollup of 7 pull requests #43216

Merged

bors added a commit that referenced this pull request Jul 14, 2017

Auto merge of #43216 - steveklabnik:rollup, r=steveklabnik

ab91c70

Rollup of 7 pull requests - Successful merges: #42926, #43125, #43157, #43167, #43187, #43203, #43204 - Failed merges:

bors merged commit a01c91c into rust-lang:master Jul 14, 2017

jonas-schievink mentioned this pull request Jul 23, 2017

Segfault in LuaTable::set mlua-rs/rlua#18

Closed

Havvy deleted the doc-path-ext branch September 27, 2017 00:42

ChrisDenton mentioned this pull request May 27, 2021

Possible errors when accessing file metadata are platform specific #85760

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document what happens on failure in path ext is_file is_dir #42926

Document what happens on failure in path ext is_file is_dir #42926

Havvy commented Jun 27, 2017

alexbowers commented Jun 27, 2017

steveklabnik commented Jun 27, 2017

steveklabnik commented Jul 5, 2017

Havvy commented Jul 5, 2017

Havvy commented Jul 5, 2017

projektir Jul 5, 2017 •

edited

Loading

GuillaumeGomez commented Jul 10, 2017

alexbowers commented Jul 10, 2017

GuillaumeGomez commented Jul 10, 2017

alexbowers commented Jul 10, 2017

steveklabnik left a comment

steveklabnik Jul 10, 2017

steveklabnik Jul 10, 2017 •

edited

Loading

Havvy commented Jul 11, 2017

GuillaumeGomez commented Jul 11, 2017

alexbowers commented Jul 11, 2017

GuillaumeGomez commented Jul 11, 2017

Havvy commented Jul 12, 2017

Mark-Simulacrum commented Jul 12, 2017

bors commented Jul 12, 2017

Mark-Simulacrum commented Jul 12, 2017

steveklabnik commented Jul 12, 2017

bors commented Jul 12, 2017

steveklabnik commented Jul 12, 2017

steveklabnik commented Jul 12, 2017

Document what happens on failure in path ext is_file is_dir #42926

Document what happens on failure in path ext is_file is_dir #42926

Conversation

Havvy commented Jun 27, 2017

alexbowers commented Jun 27, 2017

steveklabnik commented Jun 27, 2017

steveklabnik commented Jul 5, 2017

Havvy commented Jul 5, 2017

Havvy commented Jul 5, 2017

projektir Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

GuillaumeGomez commented Jul 10, 2017

alexbowers commented Jul 10, 2017

GuillaumeGomez commented Jul 10, 2017

alexbowers commented Jul 10, 2017

steveklabnik left a comment

Choose a reason for hiding this comment

steveklabnik Jul 10, 2017

Choose a reason for hiding this comment

steveklabnik Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

Havvy commented Jul 11, 2017

GuillaumeGomez commented Jul 11, 2017

alexbowers commented Jul 11, 2017

GuillaumeGomez commented Jul 11, 2017

Havvy commented Jul 12, 2017

Mark-Simulacrum commented Jul 12, 2017

bors commented Jul 12, 2017

Mark-Simulacrum commented Jul 12, 2017

steveklabnik commented Jul 12, 2017

bors commented Jul 12, 2017

steveklabnik commented Jul 12, 2017

steveklabnik commented Jul 12, 2017

projektir Jul 5, 2017 •

edited

Loading

steveklabnik Jul 10, 2017 •

edited

Loading