Re: Review Request 65811: Add `path::clean` to stout for normalizing path (for POSIX only now)

[Jie Yu]

> On March 16, 2018, 11:17 p.m., Jie Yu wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 65 (patched)
> > 
> >
> > Can you add some tests for this? This warrents some tests. Also, please 
> > reach out to Andrew because this will also compile (maybe used) on windows.

if it's posix only for now, please move to posix only headers, or add an 
assertion failure if this method is used on windows.


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65811/#review199363
---


On March 2, 2018, 7:40 p.m., Jason Lai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65811/
> ---
> 
> (Updated March 2, 2018, 7:40 p.m.)
> 
> 
> Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, 
> James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-8257
> https://issues.apache.org/jira/browse/MESOS-8257
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add `path::clean` to stout for normalizing path (for POSIX only now).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/path.hpp 
> 27438d31617b3b78bf3d4deffd25c93340610e8d 
> 
> 
> Diff: https://reviews.apache.org/r/65811/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 65811: Add `path::clean` to stout for normalizing path (for POSIX only now)

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65811/#review199363
---




3rdparty/stout/include/stout/path.hpp
Lines 65 (patched)


Can you add some tests for this? This warrents some tests. Also, please 
reach out to Andrew because this will also compile (maybe used) on windows.


- Jie Yu


On March 2, 2018, 7:40 p.m., Jason Lai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65811/
> ---
> 
> (Updated March 2, 2018, 7:40 p.m.)
> 
> 
> Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, 
> James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-8257
> https://issues.apache.org/jira/browse/MESOS-8257
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add `path::clean` to stout for normalizing path (for POSIX only now).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/path.hpp 
> 27438d31617b3b78bf3d4deffd25c93340610e8d 
> 
> 
> Diff: https://reviews.apache.org/r/65811/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 65811: Add `path::clean` to stout for normalizing path (for POSIX only now)

[Jie Yu]

> On March 2, 2018, 8:06 p.m., Chun-Hung Hsiao wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 65 (patched)
> > 
> >
> > How about s/clean/normalize/?
> 
> Jason Lai wrote:
> Indeed I considered this option. And I also considered `normpath` as in 
> `os.path.normpath` in Python but ended up picking `path::clean` from the Go 
> counterpart (`filepath.Clean`) over `path::normalize`.
> That said, I'm open to `path::normalize` if we have more folks in favor 
> of it.

+1 on using `normalize`. `clean` sounds too general to me.


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65811/#review198548
---


On March 2, 2018, 7:40 p.m., Jason Lai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65811/
> ---
> 
> (Updated March 2, 2018, 7:40 p.m.)
> 
> 
> Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, 
> James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-8257
> https://issues.apache.org/jira/browse/MESOS-8257
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add `path::clean` to stout for normalizing path (for POSIX only now).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/path.hpp 
> 27438d31617b3b78bf3d4deffd25c93340610e8d 
> 
> 
> Diff: https://reviews.apache.org/r/65811/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 65811: Add `path::clean` to stout for normalizing path (for POSIX only now)

[Jason Lai]

> On March 2, 2018, 8:06 p.m., Chun-Hung Hsiao wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 65 (patched)
> > 
> >
> > How about s/clean/normalize/?

Indeed I considered this option. And I also considered `normpath` as in 
`os.path.normpath` in Python but ended up picking `path::clean` from the Go 
counterpart (`filepath.Clean`) over `path::normalize`.
That said, I'm open to `path::normalize` if we have more folks in favor of it.


- Jason


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65811/#review198548
---


On March 2, 2018, 7:40 p.m., Jason Lai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65811/
> ---
> 
> (Updated March 2, 2018, 7:40 p.m.)
> 
> 
> Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, 
> James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-8257
> https://issues.apache.org/jira/browse/MESOS-8257
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add `path::clean` to stout for normalizing path (for POSIX only now).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/path.hpp 
> 27438d31617b3b78bf3d4deffd25c93340610e8d 
> 
> 
> Diff: https://reviews.apache.org/r/65811/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 65811: Add `path::clean` to stout for normalizing path (for POSIX only now)

[Chun-Hung Hsiao]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65811/#review198548
---




3rdparty/stout/include/stout/path.hpp
Lines 65 (patched)


How about s/clean/normalize/?


- Chun-Hung Hsiao


On March 2, 2018, 7:40 p.m., Jason Lai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65811/
> ---
> 
> (Updated March 2, 2018, 7:40 p.m.)
> 
> 
> Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, 
> James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-8257
> https://issues.apache.org/jira/browse/MESOS-8257
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add `path::clean` to stout for normalizing path (for POSIX only now).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/path.hpp 
> 27438d31617b3b78bf3d4deffd25c93340610e8d 
> 
> 
> Diff: https://reviews.apache.org/r/65811/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 65811: Add `path::clean` to stout for normalizing path (for POSIX only now)

[Jason Lai]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65811/
---

(Updated March 2, 2018, 7:40 p.m.)


Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, James 
Peach, and Zhitao Li.


Changes
---

Use `inline` for `patch::clean`


Bugs: MESOS-8257
https://issues.apache.org/jira/browse/MESOS-8257


Repository: mesos


Description
---

Add `path::clean` to stout for normalizing path (for POSIX only now).


Diffs (updated)
-

  3rdparty/stout/include/stout/path.hpp 
27438d31617b3b78bf3d4deffd25c93340610e8d 


Diff: https://reviews.apache.org/r/65811/diff/3/

Changes: https://reviews.apache.org/r/65811/diff/2-3/


Testing
---


Thanks,

Jason Lai

Re: Review Request 65811: Add `path::clean` to stout for normalizing path (for POSIX only now)

[Jason Lai]

> On Feb. 27, 2018, 5:42 p.m., Zhitao Li wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 61 (patched)
> > 
> >
> > Can you clarify whether the rules also apply to windows properly with 
> > correct separator?

Mostly it's about how to deal with the drive part of Windows pathnames. There 
is code within the same file that I can share, but I intend to do it later 
(marked with a TODO).


> On Feb. 27, 2018, 5:42 p.m., Zhitao Li wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 85 (patched)
> > 
> >
> > I don't think c++ vector supports negative index (this is not python).
> > 
> > You can use `components.back()` as long as `components` is guaranteed 
> > not empty.

Doh! This was dumb. Nice catch!


- Jason


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65811/#review198334
---


On Feb. 27, 2018, 6:52 p.m., Jason Lai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65811/
> ---
> 
> (Updated Feb. 27, 2018, 6:52 p.m.)
> 
> 
> Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, 
> James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-8257
> https://issues.apache.org/jira/browse/MESOS-8257
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add `path::clean` to stout for normalizing path (for POSIX only now).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/path.hpp 
> 27438d31617b3b78bf3d4deffd25c93340610e8d 
> 
> 
> Diff: https://reviews.apache.org/r/65811/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 65811: Add `path::clean` to stout for normalizing path (for POSIX only now)

[Jason Lai]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65811/
---

(Updated Feb. 27, 2018, 6:52 p.m.)


Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, James 
Peach, and Zhitao Li.


Changes
---

Address review comments.


Bugs: MESOS-8257
https://issues.apache.org/jira/browse/MESOS-8257


Repository: mesos


Description (updated)
---

Add `path::clean` to stout for normalizing path (for POSIX only now).


Diffs (updated)
-

  3rdparty/stout/include/stout/path.hpp 
27438d31617b3b78bf3d4deffd25c93340610e8d 


Diff: https://reviews.apache.org/r/65811/diff/2/

Changes: https://reviews.apache.org/r/65811/diff/1-2/


Testing
---


Thanks,

Jason Lai

Re: Review Request 65811: Add `path::clean` to stout for normalizing path (for POSIX only now)

[Zhitao Li]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65811/#review198334
---




3rdparty/stout/include/stout/path.hpp
Lines 61 (patched)


Can you clarify whether the rules also apply to windows properly with 
correct separator?



3rdparty/stout/include/stout/path.hpp
Lines 85 (patched)


I don't think c++ vector supports negative index (this is not python).

You can use `components.back()` as long as `components` is guaranteed not 
empty.



3rdparty/stout/include/stout/path.hpp
Lines 99 (patched)


s/slash/separator in comment



3rdparty/stout/include/stout/path.hpp
Lines 100-101 (patched)


`components.insert(components.begin(), "");`


- Zhitao Li


On Feb. 27, 2018, 6:23 a.m., Jason Lai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65811/
> ---
> 
> (Updated Feb. 27, 2018, 6:23 a.m.)
> 
> 
> Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, 
> James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-8257
> https://issues.apache.org/jira/browse/MESOS-8257
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add `path::clean` to stout for normalizing path (for POSIX only now)
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/path.hpp 
> 27438d31617b3b78bf3d4deffd25c93340610e8d 
> 
> 
> Diff: https://reviews.apache.org/r/65811/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lai
> 
>