Skip to content

File tree maxdepth#587

Merged
hulto merged 18 commits intomainfrom
file-tree-maxdepth
Dec 11, 2025
Merged

File tree maxdepth#587
hulto merged 18 commits intomainfrom
file-tree-maxdepth

Conversation

@nullmonk
Copy link
Copy Markdown
Collaborator

What type of PR is this?

/kind bug
/kind feature

What this PR does / why we need it:

Add permissions check to the file tree tome so it does not crash when encountering an unreadable directory

@nullmonk nullmonk requested a review from hulto February 13, 2024 21:14
@codecov
Copy link
Copy Markdown

codecov bot commented Feb 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.76%. Comparing base (1cad69f) to head (fdd8d60).
Report is 56 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #587      +/-   ##
==========================================
+ Coverage   70.97%   71.76%   +0.78%     
==========================================
  Files         161      175      +14     
  Lines       10776    12111    +1335     
==========================================
+ Hits         7648     8691    +1043     
- Misses       2949     3205     +256     
- Partials      179      215      +36     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Copy Markdown
Collaborator

@hulto hulto left a comment

Choose a reason for hiding this comment

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

Couple thoughts then it should be good to go

hulto
hulto previously approved these changes Feb 15, 2024
Copy link
Copy Markdown
Collaborator

@hulto hulto left a comment

Choose a reason for hiding this comment

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

Looks good to me

Not working on windows.

if isinstance(depth, str) and not depth.lstrip("-").isdigit():
print("ERROR: Invalid depth specified, must be a valid number")
return
depth=int(depth)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This seem redundant with the above

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

What do you mean redundant? -1 is not handled properly by the is digit function. This also gracefully exits rather than just crashing

@KCarretto
Copy link
Copy Markdown
Collaborator

@nullmonk / @hulto has this been fixed and tested on Windows? Let's either merge or abandon this.

Copy link
Copy Markdown
Collaborator

@hulto hulto left a comment

Choose a reason for hiding this comment

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

Gonna pull down and test

windows = sys.is_windows()

def file_list(path,tree):
def can_read(f):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Instead of adding can_read lets make file.list fail in a recoverable way similar to the new file.find function

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We'll move forward with it as is and then remove the can_read here once the eldritch change has been made.

hulto
hulto previously approved these changes Jan 26, 2025
Copy link
Copy Markdown
Collaborator

@hulto hulto left a comment

Choose a reason for hiding this comment

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

LGTM

@hulto hulto merged commit f70ad7d into main Dec 11, 2025
5 checks passed
@hulto hulto deleted the file-tree-maxdepth branch December 11, 2025 00:04
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.

3 participants