alamb commented on code in PR #546:
URL:
https://github.com/apache/arrow-rs-object-store/pull/546#discussion_r2560008656
##########
src/path/mod.rs:
##########
@@ -348,13 +398,22 @@ where
I: Into<PathPart<'a>>,
{
fn from_iter<T: IntoIterator<Item = I>>(iter: T) -> Self {
- let raw = T::into_iter(iter)
- .map(|s| s.into())
- .filter(|s| !s.raw.is_empty())
- .map(|s| s.raw)
- .join(DELIMITER);
+ let mut this = Self::ROOT;
+ this.extend(iter);
+ this
+ }
+}
- Self { raw }
+impl<'a, I: Into<PathPart<'a>>> Extend<I> for Path {
+ fn extend<T: IntoIterator<Item = I>>(&mut self, iter: T) {
Review Comment:
could you please add tests (perhaps doctests) using this feature explicitly
to help others find it?
##########
src/path/mod.rs:
##########
@@ -255,14 +269,53 @@ impl Path {
Self::parse(decoded)
}
- /// Returns the [`PathPart`] of this [`Path`]
- pub fn parts(&self) -> impl Iterator<Item = PathPart<'_>> {
- self.raw
- .split_terminator(DELIMITER)
- .map(|s| PathPart { raw: s.into() })
+ /// Returns the number of [`PathPart`]s in this [`Path`]
+ ///
+ /// This is equivalent to calling `.parts().count()` manually.
+ ///
+ /// # Performance
+ ///
+ /// This operation is `O(n)`.
+ #[doc(alias = "len")]
+ pub fn parts_count(&self) -> usize {
+ self.raw.split_terminator(DELIMITER).count()
+ }
+
+ /// True if this [`Path`] points to the root of the store, equivalent to
`Path::from("/")`.
+ ///
+ /// See also [`Path::root`].
+ ///
+ /// # Example
+ ///
+ /// ```
+ /// # use object_store::path::Path;
+ /// assert!(Path::from("/").is_root());
+ /// assert!(Path::parse("").unwrap().is_root());
+ /// ```
+ pub fn is_root(&self) -> bool {
+ self.raw.is_empty()
+ }
+
+ /// Returns the [`PathPart`]s of this [`Path`]
+ pub fn parts(&self) -> PathParts<'_> {
+ PathParts::new(&self.raw)
+ }
+
+ /// Returns a copy of this [`Path`] with the last path segment removed
+ ///
+ /// Returns `None` if this path has zero segments.
Review Comment:
this seems pretty similar to
https://doc.rust-lang.org/std/path/struct.Path.html#method.parent
Should we call this method `parent` as well to follow existing convention?
##########
src/path/mod.rs:
##########
@@ -469,6 +533,23 @@ mod tests {
assert_eq!(Path::default().parts().count(), 0);
}
+ #[test]
+ fn parts_count() {
+ assert_eq!(Path::ROOT.parts().count(), Path::ROOT.parts_count());
+
+ let path = path("foo/bar/baz");
+ assert_eq!(path.parts().count(), path.parts_count());
Review Comment:
could you also please add an assertion that the count is `3`?
##########
src/path/mod.rs:
##########
@@ -348,13 +398,22 @@ where
I: Into<PathPart<'a>>,
{
fn from_iter<T: IntoIterator<Item = I>>(iter: T) -> Self {
- let raw = T::into_iter(iter)
- .map(|s| s.into())
- .filter(|s| !s.raw.is_empty())
- .map(|s| s.raw)
- .join(DELIMITER);
+ let mut this = Self::ROOT;
+ this.extend(iter);
+ this
+ }
+}
- Self { raw }
+impl<'a, I: Into<PathPart<'a>>> Extend<I> for Path {
+ fn extend<T: IntoIterator<Item = I>>(&mut self, iter: T) {
+ for s in iter.into_iter() {
+ let s = s.into();
+ if s.raw.is_empty() {
+ continue;
+ }
+ self.raw.push(DELIMITER_CHAR);
Review Comment:
Can you add some tests showing how this works for:
1. Empty iterators
2. Iterator with a single element
3. Iterator with multiple elements
4. Extending an existing (non empty) Path? (it looks like maybe this will
add an extra `/` 🤔 )
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
