radiasoft / pykern

Apache License 2.0
5 stars 7 forks source link

fix #469: update pkio docs #470

Closed rorour closed 3 months ago

rorour commented 5 months ago

@robnagler I've realized that I can achieve what I wanted with pkio.sorted_glob. I'm confused on why we need both sorted_glob and walk_tree. We're using two different libraries to do very similar things. Can they be combined into one function with options for include_dir ~and sorting alphabetically vs top down~?

(I initially assumed that sorted_glob was sorted alphabetically. But now that I see it's also top-down I'm further confused on why we needed both. I think the documentation on sorted_glob is lacking, which is why I overlooked it initially. "sorted" is not explained at all)

robnagler commented 5 months ago

Initially, walk_tree was recursive, and sorted_glob was not. walk_tree had a filter, sorted_glob did not. They were for two different purposes.

walk_tree takes a dirname and sorted_glob takes a glob. These are different approaches. walk_tree will return dot files, for example, and sorted glob won't.

sorted_glob is only recursive with **, which was added in 7081cb4a. key sorting was added in 4328a2a8.

docstrings should refer to each other. The intentions were initially different. @e-carlin I don't remember why sorted_glob was expanded. Do you?

e-carlin commented 5 months ago

@e-carlin I don't remember why sorted_glob was expanded. Do you?

https://github.com/radiasoft/pykern/issues/123 added recursive. https://github.com/radiasoft/pykern/pull/112 added sorting on key. I don't remember the context beyond what is described in the pr/issues. Looks like walk_tree could've covered the uses cases outlined. I likely didn't know of / think to use walk tree

Sorting on key in sorted_glob is useful and a distinct use case from walk_tree. For example, here we sort on mtime where lexicographical sort would give the wrong result (11 comes before 2).

While we're updating docs the doc for walk_tree should be updated. It doesn't yield. sorted() returns a list which contains py.path.local obejcts.

rorour commented 3 months ago

@robnagler ready for review