Manchester | 25-SDC-Nov | Geraldine Edwards | Sprint 4 | Implement Shell Tools Python #259
Conversation
… for hidden files
Geraldine-Edwards
changed the title
Geraldine-Edwards
added
the
Needs Review
OracPrime
left a comment
OracPrime
left a comment
There was a problem hiding this comment.
I can't see anything obviously wrong, but there are a few bits not obviously right that I'm asking for comments on. So this review says (probably) more about my knowledge of how these linuxy things operate than yours. But code should be for the reader, not the writer, so I feel justified in making my comments!
|
|
||
|
|
||
| #set locale to the machine default setting | ||
| locale.setlocale(locale.LC_ALL, '') |
There was a problem hiding this comment.
I'm not familiar with this function. It seems odd that you're having to set something to its default. If it's the default, isn't it already set?
There was a problem hiding this comment.
Thanks for your comments David, much appreciated! So as I was trying to get my LS functionality to sort characters to display the way it mimics actual LS commands, it wasn't behaving the way I expected and so after a bit of research I found that Python doesn't use a computer's language settings by default, it defaults to the 'C' locale. Before it was sorting in ASCII order with capitals before lowercase, the real LS command uses machine's locale settings for sorting. Hope that makes sense?
There was a problem hiding this comment.
It does, and I've learned something today!
|
|
||
| # if -a flag set | ||
| if args.all: | ||
| entries.extend(['.', '..']) |
There was a problem hiding this comment.
Doing this review without running the code, so this may be rubbish. But . and .. are current and parent directory. That's not the same as adding hidden files is it? I honestly can't say this is wrong, but I need persuading it's right
There was a problem hiding this comment.
Thanks for this David :)
In my code, os.listdir() returns all files and folders in the directory, including hidden files, but it doesn't include the special directory entries (. and .. ), and since the real ls -a command always shows . and .. at the top of the list, I have to add these manually and then sort everything with locale.strxfrm, so that they output the same as the real ls -a output. :)
implement-shell-tools/wc/wc.py
Outdated
| def calculate_counts(content): | ||
| # returns a count object | ||
| return { | ||
| "lines": content.count('\n') + (1 if content and not content.endswith('\n') else 0), |
There was a problem hiding this comment.
I think the code is right, but it's a little obscure. The right of the + probably deserves an explanatory comment
There was a problem hiding this comment.
Thanks again, I will add a comment to this along the lines of "count lines by '\n'; add 1 if last line has no newline to match real wc command behavior"
:)
OracPrime
left a comment
OracPrime
left a comment
There was a problem hiding this comment.
Good work, and thanks for the education on Python locales!
OracPrime
added
Reviewed
2 participants
Learners, PR Template
Self checklist
Changelist
This PR contains the python exercises in the cat, ls and wc directories of the implement-shell-tools directory
Questions
Any advice for refactoring is welcome!