Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 76 additions & 0 deletions implement-shell-tools/cat/cat.js
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
Copy link
Member Author

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

process.stdout.write(line + '\n');
}
Comment on lines +14 to +33

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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) => {

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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
Copy link
Member Author

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 asked if there's a way to rewrite the code to not need this comment

Copy link
Collaborator

Choose a reason for hiding this comment

The 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();
52 changes: 52 additions & 0 deletions implement-shell-tools/ls/ls.js
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

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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
Copy link
Member Author

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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();
71 changes: 71 additions & 0 deletions implement-shell-tools/wc/wc.js
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

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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)?

Copy link
Collaborator

Choose a reason for hiding this comment

The 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:

description: "Within the 'printCounts' function, you use several if-statements to determine what should be in the output, but the approach means you need to keep updating 'parts' for each new possible option. Have you thought about a way to build up the list of what to print based on the enabled options, or set the default in a way that avoids having to manually check and set each option for every output?",

}

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

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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"?

Copy link
Collaborator

Choose a reason for hiding this comment

The 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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment feels unnecessary

if(multipleFiles) {
printCounts('total', totalCounts, options);
}
}
main();
Loading