-
Notifications
You must be signed in to change notification settings - Fork 2
Mirror of 376 #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| #!/usr/bin/env node | ||
| const fs = require('fs'); | ||
| const path = require('path'); | ||
|
|
||
| //shared line counter across all files(matches cat -n) | ||
| let globalLineCounter = 1; | ||
|
|
||
| function printFile(filePath, options) { | ||
| try { | ||
| const content = fs.readFileSync(filePath, 'utf-8'); | ||
| const lines = content.split('\n'); | ||
|
|
||
| lines.forEach((line) => { | ||
| if(options.numberNonEmpty) { | ||
| //-b option: number non-empty lines | ||
| if(line.trim()) { | ||
| process.stdout.write( | ||
| `${String(globalLineCounter).padStart(6)}\t${line}\n` | ||
| ); | ||
| globalLineCounter++; | ||
| } else { | ||
| process.stdout.write('\n'); | ||
| } | ||
| } else if(options.numberAll) { | ||
| //-n option: number all lines | ||
| process.stdout.write( | ||
| `${String(globalLineCounter).padStart(6)}\t${line}\n` | ||
| ); | ||
| globalLineCounter++; | ||
| } else { | ||
| //default: just print the line | ||
| process.stdout.write(line + '\n'); | ||
| } | ||
|
Comment on lines
+14
to
+33
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What differences and similarities do you notice in how each option prints lines? How might you avoid repeating the printing and formatting logic?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I left a similar comment, but with a little more detail |
||
| }); | ||
|
|
||
| } catch (error) { | ||
| console.error(`Error reading file ${filePath}: ${error.message}`); | ||
| } | ||
| } | ||
|
|
||
| function main() { | ||
| const args = process.argv.slice(2); | ||
| const options = { | ||
| numberNonEmpty: false, | ||
| numberAll: false, | ||
| }; | ||
| const filePatterns = []; | ||
|
|
||
| args.forEach((arg) => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While it is possible to build an argument parser like this, my approach would be to prefer an argument parser specific library. Is the ai bot set up to recognise situations like this? I guess it is tricky because we don't want it to just suggest replacing the entire file with (in this case) a call to an existing cat tool, but using something like commander's program API might make this more readable.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it depend on the goal of the assignment whether trainee supposed to build something from scratch or learn to use existing tools. Ai bot doesn't set up to give feedback on such situations. In this particular task I believe trainees should use commander, since it's covered in the prep for this backlog.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, the bot is (currently) doing context-free reviews without knowing the assignments - if we want to include "are you solving the correct problem" style reviews, I'd punt that to a future feature request :) |
||
| if(arg === '-n') { | ||
| options.numberAll = true; | ||
| } else if(arg === '-b') { | ||
| options.numberNonEmpty = true; | ||
| } else { | ||
| filePatterns.push(arg); | ||
| } | ||
| }); | ||
| // -b takes precedence over -n | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe the tool could have asked if there's a way to rewrite the code to not need this comment
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting, I haven't thought about this, thanks for pointing it out |
||
| if(options.numberNonEmpty) { | ||
| options.numberAll = false; | ||
| } | ||
|
|
||
| if(filePatterns.length === 0) { | ||
| console.log("cat: missing file operand"); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| const files = filePatterns; | ||
|
|
||
| files.forEach((file) => { | ||
| const resolvedPath = path.resolve(process.cwd(), file); | ||
| printFile(resolvedPath, options); | ||
| }); | ||
| } | ||
|
|
||
| main(); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| #!/usr/bin/env node | ||
| const fs = require('node:fs'); | ||
| const path = require('node:path'); | ||
|
|
||
| function listDirectory(dirPath, showAll, onePerLine) { | ||
| try { | ||
| const entries = fs.readdirSync(dirPath, { withFileTypes: true }); | ||
| const filtered = entries.filter((entry) => showAll || !entry.name.startsWith('.')); | ||
|
|
||
| if(onePerLine) { | ||
| filtered.forEach(entry => console.log(entry.name)); | ||
| } else { | ||
| const names = filtered.map(entry => entry.name); | ||
| console.log(names.join(' ')); | ||
| } | ||
| } catch (error) { | ||
| console.error(`Error reading directory ${dirPath}: ${error.message}`); | ||
| } | ||
| } | ||
| function main() { | ||
| const args = process.argv.slice(2); | ||
| // Check for options | ||
| const showAll = args.includes('-a'); | ||
| const onePerLine = args.includes('-1'); | ||
|
Comment on lines
+23
to
+24
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If someone else reads this code, would it be immediately obvious what 'showAll' or 'onePerLine' do? Is there a name you could choose that would make their purpose even clearer?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Droid-An These variable names look pretty reasonable to me as-is - WDYT?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also think the naming is fine - this feedback seems to be just informational. In the Coolify logs for the deployed app, the AI review output is logged, and this point is classified as 1 on a scale from 1 to 5, while the other points are classified as 2.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, let's maybe filter out informational things. When we have quality tracking in place, we could try letting in some % of informational comments to see if people find them useful (maybe leave 10% of informational comments, but gather feedback on which are useful). But this would be a future enhancement, not part of the initial project :) |
||
| //remove options from args | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment feels unnecessary |
||
| const directories = args.filter(arg => arg !== '-a' && arg !== '-1'); | ||
|
|
||
| // If no directory is specified, list the current directory | ||
| if(directories.length === 0) { | ||
| listDirectory(process.cwd(), showAll, onePerLine); | ||
| return; | ||
|
Comment on lines
+29
to
+31
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you think of a way to remove this duplication so that the function for listing directories is always called in the same way, regardless of how many directories or files are provided?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I left a similar comment to this 👍 |
||
| } | ||
| //If a directory is specified, list that directory | ||
| directories.forEach((arg, index) => { | ||
| try { | ||
| const stats = fs.statSync(arg); | ||
| if(stats.isDirectory()) { | ||
| //Print header if multiple directories are listed | ||
| if(directories.length > 1) console.log(`${arg}:`); | ||
|
|
||
| listDirectory(arg, showAll, onePerLine); | ||
| //add a blank line between directory listings if there are multiple directories | ||
| if(directories.length > 1 && index < directories.length - 1) console.log(''); | ||
|
Comment on lines
+34
to
+43
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you think of a way to remove this duplication so that the function for listing directories is always called in the same way, regardless of how many directories or files are provided?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Droid-An This code looks pretty reasonable to me - do you know what it's trying to nudge towards?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From the logs, it looks like this comment refers to the same point as the one above. I assume it simply specifies the lines where the problematic code appears. Since listDirectories is called twice, the ai posted two comments.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, consolidating these down into just one comment (either at the first location, or in a line range that covers both) feels more useful? |
||
| } else{ | ||
| console.log(arg);// single file | ||
| } | ||
| } catch (error) { | ||
| console.error(`Error accessing ${arg}: ${error.message}`); | ||
| } | ||
| }); | ||
| } | ||
| main(); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| #!/usr/bin/env node | ||
| const fs = require('node:fs'); | ||
| // Function to count lines, words, and bytes in a file | ||
| function countFileContent(content) { | ||
| const lines = content.split('\n').length; // Count lines by splitting on newline characters | ||
| const words = content.trim().split(/\s+/).filter(Boolean).length; // Split by whitespace and filter out empty strings | ||
| const bytes = Buffer.byteLength(content, 'utf8'); | ||
| return { lines, words, bytes }; | ||
| } | ||
|
|
||
| //print counts in the format of wc according to options | ||
| function printCounts(filePath, counts, options) { | ||
| const parts = []; | ||
| if(options.line) parts.push(counts.lines); | ||
| if(options.word) parts.push(counts.words); | ||
| if(options.byte) parts.push(counts.bytes); | ||
| //if no specific count options are provided, print all counts | ||
| if(!options.line && !options.word && !options.byte) { | ||
| //default is to print all counts | ||
| parts.push(counts.lines, counts.words, counts.bytes); | ||
| } | ||
| console.log(parts.join('\t'),filePath); | ||
|
Comment on lines
+13
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How would you design the code so that adding future options for 'wc' outputs would not require modifying existing code for every new output?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Droid-An I'm not sure this comment is specific enough? I guess this is trying to get at the same thing as CodeYourFuture/Module-Tools#376 (comment)?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's telling about lots of if else statements. Here is more detailed description of this issue from ai logs:
|
||
| } | ||
|
|
||
| function main() { | ||
| const args = process.argv.slice(2); | ||
| const options = { | ||
| line: false, | ||
| word: false, | ||
| byte: false, | ||
| }; | ||
|
|
||
| //Separate options from file paths | ||
| const files = []; | ||
| args.forEach((arg) => { | ||
| if(arg === '-l') options.line = true; | ||
| else if(arg === '-w') options.word = true; | ||
| else if(arg === '-c') options.byte = true; | ||
| else files.push(arg); | ||
| }); | ||
|
|
||
| if(files.length === 0) { | ||
| console.error('No files specified'); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| let totalCounts = { lines: 0, words: 0, bytes: 0 }; | ||
| const multipleFiles = files.length > 1; | ||
|
|
||
| files.forEach(file => { | ||
| try { | ||
| const content = fs.readFileSync(file, 'utf-8'); | ||
| const counts = countFileContent(content); | ||
|
|
||
| // Sum counts for total if multiple files | ||
| totalCounts.lines += counts.lines; | ||
| totalCounts.words += counts.words; | ||
| totalCounts.bytes += counts.bytes; | ||
|
Comment on lines
+47
to
+58
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What might happen to the 'totalCounts' object if you only have one file? Does it ever get used? How could you make updating totals conditional on having more than one file?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Droid-An This comment feels contradictory - before the comments were saying "try not to special case the single-file case" and now it's saying "try to special case the single-file case"?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, I agree. I think I need to promt ai to be more consistent |
||
|
|
||
| printCounts(file, counts, options); | ||
| } catch (error) { | ||
| console.error(`Error reading file ${file}: ${error.message}`); | ||
| } | ||
| }); | ||
|
|
||
| // If multiple files, print total counts | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment feels unnecessary |
||
| if(multipleFiles) { | ||
| printCounts('total', totalCounts, options); | ||
| } | ||
| } | ||
| main(); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the tool could have noticed this comment isn't very useful