Skip to content
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

Add system path check to start command #4

Merged
merged 5 commits into from
Sep 28, 2023
Merged

Add system path check to start command #4

merged 5 commits into from
Sep 28, 2023

Conversation

zoujas
Copy link
Contributor

@zoujas zoujas commented Sep 28, 2023

Created a new file: lib/commands/pathdetect.mjs that contains a function to check if current working directory is a common system directory.

Edited lib/commands/init.mjs to print a warning to console if user is in current working dir, and exit with err code 1.

Copy link
Contributor

@ericbeland ericbeland left a comment

Choose a reason for hiding this comment

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

In general, great work! Please address the feedback items I mentioned, then push up your changes, which will update the PR.

@@ -3,6 +3,19 @@ import fs from 'fs';
import path from 'path';
import ejs from 'ejs';
import {getAbsolutePathFromRelativeToRoot} from "../utils/path_utils.mjs";
import {isSystemDirectory} from "../commands/pathdetect.mjs";
Copy link
Contributor

Choose a reason for hiding this comment

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

For filenames, we have a convention of separating words with underscores, so we probably want path_detect.mjs here.

import {isSystemDirectory} from "../commands/pathdetect.mjs";
import { fileURLToPath } from 'url';
import { dirname } from 'path';
const __filename = fileURLToPath(import.meta.url);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the variables are private because we are in a module, so no need to underscore them in the names.

const __dirname = dirname(__filename);

let isSDirectory = isSystemDirectory(__dirname);
console.log(isSDirectory);
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably don't want to keep this in the output, so we should clean that line up.

dirPath = dirPath.toLowerCase().replace(/\\+/g, '/');
if (!dirPath.endsWith('/')) {
dirPath += '/';
console.log(dirPath)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this meant to be just debug output, or were you thinking this should stay in our output?

Copy link
Contributor

@ericbeland ericbeland left a comment

Choose a reason for hiding this comment

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

👍

@zoujas zoujas merged commit 2bc922e into master Sep 28, 2023
0 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants