London | 26-SDC-Mar | Khilola Rustamova| Sprint 4 |Implement shell tools python#537
London | 26-SDC-Mar | Khilola Rustamova| Sprint 4 |Implement shell tools python#537HilolaRustam wants to merge 10 commits into
Conversation
HilolaRustam
added
Needs Review
illicitonion
added
Review in progress
illicitonion
left a comment
illicitonion
left a comment
There was a problem hiding this comment.
This is generally looking good, and like things work :) I left a few style comments, and many of my comments to one project apply to the others as well.
illicitonion
added
Reviewed
HilolaRustam
added
Needs Review
illicitonion
left a comment
illicitonion
left a comment
There was a problem hiding this comment.
This looks really good, well done!
| for a in args: | ||
| if a == "-n": | ||
| number_all = True | ||
| elif a == "-b": | ||
| number_nonempty = True | ||
| else: | ||
| paths.append(a) | ||
|
|
||
| if number_nonempty: | ||
| number_all = False |
There was a problem hiding this comment.
This kind of relationship where different options/states are related but exclusive can be hard to follow. (Here, it should never be the case that number_all and number_nonempty are both True - it's an invalid state in the program).
Instead of this, we sometimes use enums for this (or can use strings as enums). Consider a single variable number_mode which could be set to either "none", "non_empty" or "all" - here we don't need to think about what both True mean - we just have one variable which could have one of three variables. What do you think about this?
There was a problem hiding this comment.
I don't think you addressed this comment?
illicitonion
changed the title
illicitonion
added
Reviewed
HilolaRustam
added
Needs Review
illicitonion
removed
the
Needs Review
illicitonion
left a comment
illicitonion
left a comment
There was a problem hiding this comment.
This is generally looking really good - just a couple of last things :)
| sys.exit(1) | ||
|
|
||
|
|
||
| def expand_paths(paths): |
There was a problem hiding this comment.
You shouldn't need to do this expansion in your program; the shell should do it for you.
| for a in args: | ||
| if a == "-n": | ||
| number_all = True | ||
| elif a == "-b": | ||
| number_nonempty = True | ||
| else: | ||
| paths.append(a) | ||
|
|
||
| if number_nonempty: | ||
| number_all = False |
There was a problem hiding this comment.
I don't think you addressed this comment?
illicitonion
added
the
Reviewed
2 participants
Learners, PR Template
Self checklist
Changelist
implementation of cat, ls and wc with python