Glasgow | 25-SDC-NOV | Katarzyna Kazimierczuk | Sprint 4 | Python shell tools#331
Glasgow | 25-SDC-NOV | Katarzyna Kazimierczuk | Sprint 4 | Python shell tools#331katarzynakaz wants to merge 2 commits intoCodeYourFuture:mainfrom
Conversation
katarzynakaz
added
Needs Review
This comment has been minimized.
This comment has been minimized.
github-actions
bot
removed
the
Needs Review
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
katarzynakaz
added
Needs Review
OracPrime
added
the
Review in progress
OracPrime
left a comment
OracPrime
left a comment
There was a problem hiding this comment.
Some significant cleaning up needed on this one.
|
|
||
| for file in listOfFiles: | ||
| grabbedText = Path(file).read_text(encoding="utf-8") | ||
| splitLines = grabbedText.split("\n") |
There was a problem hiding this comment.
There's a subtle bug here I think. Consider what happens if the file ends with a line which ends with a newline.
Or to put it another way, what does "Hello\n".split("\n") return?
| flag = None | ||
| restIsFiles = args | ||
|
|
||
| def takeSpecifiedAction(cleanLinesArr, flag): |
There was a problem hiding this comment.
It's bad form, normally, to hide functions inside the top-level code of a python file. This should be before line 14.
| def takeSpecifiedAction(cleanLinesArr, flag): | ||
| countingOnlyFullLines = 1 | ||
|
|
||
| for file in cleanLinesArr: |
There was a problem hiding this comment.
"file" isn't a file here, it's a line. You need to rename the variable
| print(f"{countingOnlyFullLines} {file}") | ||
| countingOnlyFullLines += 1 | ||
| else: | ||
| print("incorrect flag") |
There was a problem hiding this comment.
You should validate the flag outside the loop. At the moment if the flag is invalid you'll print this message for every line of every file.
|
|
||
| # `ls -1 sample-files` | ||
| def showVisibleInSampleFiles(): | ||
| listOfFiles = os.listdir("sample-files") |
|
|
||
| if "-a" in argv: | ||
| showAllInSampleFiles() | ||
| elif "sample-files" in argv: |
There was a problem hiding this comment.
No - you shouldn't be worrying about sample-files as a special case. You should take the arguments from the command line! I suspect if you look back at this file now you have more experience you'll be reaching for the keyboard!
| from pathlib import Path | ||
|
|
||
| def calculateCounts(inputFiles): | ||
| return { |
There was a problem hiding this comment.
This is counting lines, words and bytes every time, which is inefficient. You should pass in a flag as to what you want, and just do a single calculation
|
|
||
| def calculateCounts(inputFiles): | ||
| return { | ||
| "lines": len(inputFiles.split("\n")) - 1, |
There was a problem hiding this comment.
split("\n") has the same bug discussed in cat.py
| print(f"{counts['words']} {file}") | ||
|
|
||
| # * `wc -c sample-files/3.txt` | ||
| def countBytes(listOfFiles): |
There was a problem hiding this comment.
These three functions could easily be combined into a single function with a flag.
|
|
||
| Convert the decimal number 14 to binary. | ||
| Answer: | ||
| Answer:1110 |
There was a problem hiding this comment.
Isn't this file left over from a different sprint? And we need .gitignore for .DS_Store
OracPrime
added
Reviewed
|
I also don't understand why this has the "NotCoursework" tag - it's in the backlog |
2 participants
Self checklist
Changelist
Cat, wc and ls implemented in pyton.