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 14 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
HilolaRustam
added
Needs Review
illicitonion
left a comment
illicitonion
left a comment
There was a problem hiding this comment.
Sorry for the huge delay here. This is looking really good - a couple of last things!
| if number_mode == "non_empty": | ||
| if not is_empty: | ||
| prefix = f"{line_no:6}\t" | ||
| line_no += 1 | ||
| else: | ||
| prefix = "" | ||
| elif number_mode=="all": | ||
| prefix = f"{line_no:6}\t" | ||
| line_no += 1 |
There was a problem hiding this comment.
These branches look very similar - could they be combined?
| parts = [] | ||
| if show_l: | ||
| parts.append(f"{l:3}") | ||
| if show_w: | ||
| parts.append(f"{w:4}") | ||
| if show_c: | ||
| parts.append(f"{c:4}") | ||
|
|
||
|
|
||
| print("".join(parts) + " " + file) | ||
|
|
||
| # print total if multiple files | ||
| if len(results) > 1: | ||
| parts = [] | ||
|
|
||
| if show_l: | ||
| parts.append(f"{total_l:3}") | ||
| if show_w: | ||
| parts.append(f"{total_w:4}") | ||
| if show_c: | ||
| parts.append(f"{total_c:4}") |
There was a problem hiding this comment.
These blocks feel quite repetitive. Can you work out how to deduplicate them?
One thing that may be useful to know is you can define nested functions which can access any variables within the scope they're defined in. e.g. you can write:
def outer():
value = 1
def inner(name):
print(f"{name}: {value}")
for letter in ["a", "b", "c"]:
inner(letter)and inner can access value.
illicitonion
added
Reviewed
2 participants
Learners, PR Template
Self checklist
Changelist
implementation of cat, ls and wc with python