Repository navigation
London | 26-SDC-July | Raihan Sharif | Sprint 4 | implement shell tools python - #649
RaihanSharif wants to merge 22 commits into
Conversation
all paths that are not accessible as a file/folder throw same error
d5a3942 to
87d8179
Compare
SlideGauge
left a comment
There was a problem hiding this comment.
Good job, I left several comments, could you fix them please?
| prog="a simple version of wc. Takes in one or more files.", | ||
| description="ls command line tool which can accept -l -w -c cflags") | ||
|
|
||
| parser.add_argument("-l", action="store_true", help="show line count", default="l") |
There was a problem hiding this comment.
Each flag is declared action="store_true", default="l" (and "w", "c"). With store_true, when the flag is
absent the value falls back to that default string - and a non-empty string is truthy. So run wc -l
sample-files/3.txt: does it print only the line count, or all three? What should default be for a boolean flag so
that "flag not given" reads as false?
There was a problem hiding this comment.
...uh, yeah, that's bad.
I've now added a check that if no flags are supplied, then just set them all to true.
Which then means that wc prints the lines, word count, and bytes of the files.
Otherwise, the flags are processed individually, so that any combination of the three will work.
| # {some_str:6} left justifed string, length fo at least 6 | ||
| output.append(f"{line_num:6}\t{line}") | ||
| elif number_all: | ||
| for i, line in enumerate(lines, start=1): |
There was a problem hiding this comment.
format_lines numbers with enumerate(..., start=1) and is called fresh for each file in cat_file. Run cat -n
sample-files/*.txt against the real cat - does the count reset to 1 at each file, or keep climbing? Where would the counter need to live to carry across files?
There was a problem hiding this comment.
yes, the numbering starts from 1 for each file in the cat input
cat -n sample-files/*
1 Once upon a time...
1 There was a house made of gingerbread.
1 It looked delicious.
2 I was tempted to take a bite of it.
3 But this seemed like a bad idea...
4
5 There's more to come, though...
python cat:
python3 cat.py -n sample-files/*
1 Once upon a time...
1 There was a house made of gingerbread.
1 It looked delicious.
2 I was tempted to take a bite of it.
3 But this seemed like a bad idea...
4
5 There's more to come, though...
You asked this in the nodeJS version of cat as well. Are you hinting at something, and I'm just not getting it?
There was a problem hiding this comment.
Depending on how your system is setup you might not see this.
I think that what the original reviewer is hinting at is, what if you wanted all the numbers for multiple files in sequence? On my machine, it looks like this:
cat -n sample-files/*
1 Once upon a time...
2 There was a house made of gingerbread.
3 It looked delicious.
4 I was tempted to take a bite of it.
5 But this seemed like a bad idea...
6
7 There's more to come, though...
There was a problem hiding this comment.
Hi Leon,
I did initially implement cat so that the numbering would be across files. I changed it to reflect the behaviour of my macOS/BSD version of cat which does actually restart numbering for each file it reads from stdin.
This is what it looks like
/bin/cat -n sample-files/1.txt sample-files/2.txt sample-files/3.txt
1 Once upon a time...
1 There was a house made of gingerbread.
1 It looked delicious.
2 I was tempted to take a bite of it.
3 But this seemed like a bad idea...
4
5 There's more to come, though...
I have changed it to do numbering across files. I just pass in a line_num variable (initially set to 1) to the format_lines function, which then increments line_num and returns a list of lines formatted with numbers, and the then the cat_file function increases line_num by the number of lines returned, and calls format_lines on the next file it reads.
This does require a global line_num variable, but it could just easily all be implemented inside of cat_file. It would just change from taking a single path, to taking a list of paths.
| lines = file.readlines() | ||
|
|
||
| if (args.l): | ||
| if (len(lines) > 0 and lines[-1] == ""): |
There was a problem hiding this comment.
lines[-1] == "" - but readlines() keeps the trailing \n, so an element is "\n", never "". Does this pop ever
actually run? Check wc -l against real wc on a file that does and doesn't end in a newline.
There was a problem hiding this comment.
Thank you for pointing that out, I 'translated' my js code too literally without accounting for how readlines() or how wc actually counts lines.
wc counts the number of '\n' characters, not necessarily non-empty lines. Last empty line is excluded because it doesn't have a '\n' in it.
I've changed my implementation to just read the file, and count the number of '\n' characters. The output now matches the real wc.
It also made my -w logic a bit simpler.
LonMcGregor
left a comment
There was a problem hiding this comment.
Apologies for the delay in continuing this review.
This looks almost complete, I've left a couple of final comments
| lines = content.split('\n') | ||
|
|
||
| if (args.l): | ||
| #if (len(lines)) > 0: |
There was a problem hiding this comment.
If this code isn't needed anymore, you can remove it entirely instead of commenting. Git will keep a history of old lines if you needed it.
| # {some_str:6} left justifed string, length fo at least 6 | ||
| output.append(f"{line_num:6}\t{line}") | ||
| elif number_all: | ||
| for i, line in enumerate(lines, start=1): |
There was a problem hiding this comment.
Depending on how your system is setup you might not see this.
I think that what the original reviewer is hinting at is, what if you wanted all the numbers for multiple files in sequence? On my machine, it looks like this:
cat -n sample-files/*
1 Once upon a time...
2 There was a house made of gingerbread.
3 It looked delicious.
4 I was tempted to take a bite of it.
5 But this seemed like a bad idea...
6
7 There's more to come, though...
Learners, PR Template
Self checklist
Changelist
Implemented cat, ls, wc.
Task code: CYF-1152