-
Notifications
You must be signed in to change notification settings - Fork 17
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
Code clean up #1
Conversation
On main(): - Remove blank likes - Update the order of the arguments to DirectoryTree constructor On parse_cmd_line_arguments(): - Remove unneeded upper case letter from strings - Move the version attribute up - Remove the unneeded type argument because str is the default type for command-line arguments
- Add a missing import - Add a new module-level constant to provide a new prefix On DirectoryTree - Swap the order of the argument in the class initializer - Update functions calls and rename item to entry On _TreeGenerator - Rename the first argument of the initializer to be more consistent with the current API - Turn .dir_only into a non-public attribute - Use a deque instead of a list to optimize the insertions of item at the beginning of the data structure - Rename the dir argument to directory so we can avoid name collisions with the built-in dir() function - Remove the path local variable joining - Rename path to entry so we maintain a uniform naming - Move the path argument to the beginning of the argument in _add_file(). Also, it was renamed from path to file
rptree/rptree.py
Outdated
return entries | ||
entries = sorted(entries, key=lambda e: dir.joinpath(e).is_file()) | ||
entries = sorted(entries, key=lambda entry: entry.is_file()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we already have entries
we could change this to an in-place sort:
entries.sort(key=lambda entry: entry.is_file())
But that's just a nitpick, fine by me either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we could do that but entries
is a generator.
There was a problem hiding this 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 @lpozo!
README.md
Outdated
2. Run the application | ||
|
||
```sh | ||
(venv) $ python tree.py /path/to/directory/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to keep the module name as tree
or change it to rptree
?
Would be kind of nice to (eventually) just do a pip install rptree
and then calling it via $ rptree ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the idea. I think I'll remove this script and create a __main__.py
module in rptree
to provide the entry point script.
Idea: You could include an example of the output generated by the tool. I think that'd be interesting for people to see here on GitHub and on PyPI when they click on the package. |
This is intended to clean up the code and improve readability. See commit messages for details.