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

Fix error message for WindowsPath::new #16443

Merged
merged 1 commit into from
Aug 21, 2014

Conversation

steveklabnik
Copy link
Member

///
/// # Example
///
/// ```{rust,ignore}
Copy link
Member

Choose a reason for hiding this comment

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

Why ignore?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because we can't guarantee this path exists, right?

though, I guess, in this situation, it won't fail. haha 😅

Copy link
Member

Choose a reason for hiding this comment

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

A Path is an abstract object, it doesn't need to represent the actual file system (unless you call a function like is_dir() or whatever on it).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, makes total sense, it was just my brain being silly.

@steveklabnik
Copy link
Member Author

This has been updated to just improve style. r?

#[inline]
pub fn new<T: BytesContainer>(path: T) -> Path {
GenericPath::new(path)
}

/// Returns a new Path from a byte vector or string, if possible
/// Returns a new `Some(Path)` from a `BytesContainer`. Returns `None` if the vector contains a
/// null byte, or if it contains invalid UTF-8.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a rather long summary line. I would suggest breaking the two sentences apart and using only the first as the summary. When a summary says that it returns Some(...) I think it's reasonable to expect that people will understand that the description explains the conditions under which it returns None.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed.

@lilyball
Copy link
Contributor

Is there something wrong with the term "NUL"? That's the name of the character '\x00'. "null byte" to me sounds weird, because "null" typically means the zero value for some reference type, and "byte" is not a reference type.

@lilyball
Copy link
Contributor

Also the commit message and PR title should probably be updated to say WindowsPath::new, as this doesn't touch PosixPath::new.

@steveklabnik
Copy link
Member Author

I was not familiar with NUL being the technical term of \x00. I will change it back.

@steveklabnik steveklabnik changed the title Fix error message for Path::new Fix error message for WindowsPath::new Aug 18, 2014
@steveklabnik
Copy link
Member Author

Updated, @kballard

@steveklabnik
Copy link
Member Author

@kballard bors thinks this is 'under discussion.' Any ideas?

@lilyball
Copy link
Contributor

@steveklabnik Apparently I r+'d the copy of the commit in rust-lang/rust and not the copy in steveklabnik/rust. GitHub makes that easy to accidentally do, and doesn't make a distinction when showing the comments in the discussion, but bors cares.

@steveklabnik
Copy link
Member Author

Noted!

@lilyball
Copy link
Contributor

make tidy error, trailing whitespace on line 636.

@steveklabnik
Copy link
Member Author

zomg, fixing

@steveklabnik
Copy link
Member Author

@kballard updated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants