Skip to content

Add ESLint configuration and refactor grefplus to use ES modules #169

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.

Sign up for GitHub

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
+3,627 −3,592
Open

Add ESLint configuration and refactor grefplus to use ES modules #169

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
751ab5c
Refactor grefplus to use ES modules and update alias paths
begin-again Oct 15, 2024
628cd6c
Update ESLint configuration for ES module support and bump version to…
begin-again Oct 15, 2024
26ad0bc
Refactor date validation logic in cmdline.mjs to use a reusable function
begin-again Oct 15, 2024
97a6905
Update GitHub Actions workflows to use 'yarn install --immutable' for…
begin-again Oct 17, 2024
ed325f1
Update GitHub Actions workflow to enable corepack and use 'yarn insta…
begin-again Oct 17, 2024
08d52cf
Refactor code structure for improved readability and maintainability
begin-again Oct 17, 2024
73d53f1
Enable corepack in GitHub Actions workflow and update yarn install co…
begin-again Oct 17, 2024
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
8 changes: 8 additions & 0 deletions .eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,14 @@ module.exports = {
, mocha: true
, es2020: true
}
, overrides: [
{
files: [ '*.mjs' ]
, parserOptions: {
sourceType: 'module'
}
}
]
, rules: {
'array-bracket-spacing': [ 'error', 'always' ]
, 'arrow-spacing': [ 'error', { 'before': true, 'after': true } ]
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/node.nix.js.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,5 @@ jobs:
- uses: actions/setup-node@v4
with:
node-version: ${{ matrix.node-version }}
- run: yarn install --frozen-lockfile
- run: yarn install ---immutable
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

Yarn is not configured in the workflow

The workflow currently lacks steps to install or configure Yarn, which results in the yarn commands failing. To ensure Yarn is available and correctly set up in the CI environment, please consider the following changes:

  1. Enable Corepack and Set Yarn Version:

    - uses: actions/setup-node@v4
+ uses: actions/setup-node@v4
+ - run: corepack enable
+ - run: yarn set version stable

  2. Correct the Install Command Flag:

    - - run: yarn install ---immutable
+ - run: yarn install --immutable

These adjustments will ensure that Yarn is properly installed and configured, and that the correct flags are used during dependency installation.

🔗 Analysis chain

Verify Yarn version and correct flag usage

The change from --frozen-lockfile to ---immutable suggests a migration from Yarn 1.x to Yarn 2+ (Berry). This is a good practice as it ensures more deterministic builds. However, there are two points to consider:

  1. The use of three dashes (---) for the immutable flag is unusual. Typically, long-form flags use two dashes (--). This might be a typo and should be corrected to --immutable.

  2. The workflow doesn't explicitly specify the Yarn version. To ensure consistency across different environments and prevent potential issues, it's recommended to pin the Yarn version.

Consider applying the following changes:

  1. Correct the flag:
- - run: yarn install ---immutable
+ - run: yarn install --immutable
  1. Add a step to set up a specific Yarn version before the install step:
- uses: actions/setup-node@v4
  with:
    node-version: ${{ matrix.node-version }}
+ - run: corepack enable
+ - run: yarn set version stable
- run: yarn install --immutable

To confirm the correct Yarn version and available flags, you can run:

This will help verify the correct Yarn version is being used and the available flags for the install command.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check Yarn version and available flags
yarn --version
yarn install --help

Length of output: 182

Script:

#!/bin/bash
# Search for Yarn installation or setup steps in the workflow file
grep -iE 'yarn install|yarn set version|actions/setup-node' .github/workflows/node.nix.js.yml

Length of output: 170

Script:

#!/bin/bash
# Search for Yarn setup steps in the workflow file
grep -iE 'corepack enable|yarn set version|yarn config|yarn init' .github/workflows/node.nix.js.yml

Length of output: 101

- run: yarn test
2 changes: 1 addition & 1 deletion .github/workflows/node.win.js.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,5 @@ jobs:
- uses: actions/setup-node@v4
with:
node-version: ${{ matrix.node-version }}
- run: yarn install --frozen-lockfile
- run: yarn install --immutable
- run: yarn test
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "my-dev-tools",
"version": "4.0.0",
"version": "5.0.0-beta.0",
"main": "",
"repository": "https://github.com/begin-again/dev-tools.git",
"license": "MIT",
Expand All @@ -27,7 +27,7 @@
"chai": "^4.5.0",
"eslint": "^8.57.0",
"eslint-plugin-node": "^11.1.0",
"mocha": "^10.6.0",
"mocha": "^10.7.3",
"mock-fs": "^5.4.0",
"nyc": "^17.1.0",
"proxyquire": "^2.1.3",
Expand Down
2 changes: 1 addition & 1 deletion src/alias.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ alias ll='ls -ltrh'
## tools
alias resource='source $HOME/.bashrc'
alias clean='node $DEVROOT/dev-tools/src/clean/index.js'
alias grefp='node $DEVROOT/dev-tools/src/grefplus'
alias grefp='node $DEVROOT/dev-tools/src/grefplus/index.mjs'
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Incorrect path in grefp alias due to unset $DEVROOT.

  • Update the alias in src/alias.sh to: 
    alias grefp='node ./src/grefplus/index.mjs'
🔗 Analysis chain

LGTM! The alias update aligns with the PR objectives.

The change from grefp='node $DEVROOT/dev-tools/src/grefplus' to grefp='node $DEVROOT/dev-tools/src/grefplus/index.mjs' correctly reflects the transition to ES modules as mentioned in the PR objectives.

To ensure this change doesn't introduce breaking changes, please run the following verification:

This script will verify the existence of the new file, check for ES module syntax, and attempt to run the grefp command.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the existence of the new grefplus index.mjs file and check its content

# Test 1: Check if the new file exists
if [ -f "$DEVROOT/dev-tools/src/grefplus/index.mjs" ]; then
    echo "index.mjs file exists."
else
    echo "Error: index.mjs file not found!"
    exit 1
fi

# Test 2: Check if the file contains ES module syntax
if grep -q "export" "$DEVROOT/dev-tools/src/grefplus/index.mjs"; then
    echo "index.mjs contains ES module export syntax."
else
    echo "Warning: index.mjs might not be using ES module syntax."
fi

# Test 3: Try running the grefp command with --help to check if it's working
if grefp --help > /dev/null 2>&1; then
    echo "grefp command executed successfully."
else
    echo "Error: grefp command failed to execute."
    exit 1
fi

Length of output: 130

Script:

#!/bin/bash
# Description: Output the value of the DEVROOT environment variable

echo "DEVROOT is set to: $DEVROOT"

Length of output: 48

Script:

#!/bin/bash
# Description: Search for the index.mjs file in the repository

fd "index.mjs" .

Length of output: 41

alias branches='node $DEVROOT/dev-tools/src/branches'
alias nt='node $DEVROOT/dev-tools/src/node-tools'
alias yn='node $DEVROOT/dev-tools/src/yarn'
Expand Down
110 changes: 60 additions & 50 deletions src/grefplus/cmdline.js → src/grefplus/cmdline.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
const { accessSync, constants, lstatSync } = require('fs');
const yargs = require('yargs');
const { DateTime } = require('luxon');
import { accessSync, constants, lstatSync } from 'node:fs';
import yargs from 'yargs/yargs';
import { DateTime } from 'luxon';

const options = {
dateOptions: 'yyyy-MM-dd hh:mm:ss a'
Expand Down Expand Up @@ -32,8 +32,7 @@ const cmdKeys = {
alias: 'r'
, describe: 'root folder of development environment (/c/blah/blah). Default is DEVROOT'
, type: 'array'
// eslint-disable-next-line no-process-env
, default: process.env.DEVROOT
, default: [process.env.DEVROOT]
}
, 'date': {
alias: 'd'
Expand All @@ -46,13 +45,14 @@ const cmdKeys = {
/**
* Validates from and to dates,
*
* @param {String} checkDate
* @param {string} checkDate
* @param {string} msg
* @throws if date is not valid
* @private
*/
const validateDate = (checkDate, msg) => {
try {
const parsed = DateTime.fromFormat(checkDate, options.allowedFormat);
const parsed = DateTime.fromFormat(checkDate.toString(), options.allowedFormat);
if(!parsed.isValid) {
throw new Error('unknown format');
}
Expand All @@ -69,11 +69,11 @@ const validateDate = (checkDate, msg) => {
/**
* Aborts build if dev root path does not exist
*
* @param {String} devRoot
* @param {string[]} devRoot
* @throws if path not accessible
* @private
*/
const validatePath = ({ devRoot }) => {
const validatePath = (devRoot) => {
let problematicRoot = null;

devRoot.forEach(root => {
Expand Down Expand Up @@ -108,24 +108,47 @@ const isFuture = (checkDate) => {
return _date > endOfToday;
};


/**
*
* @param {string} argName
* @param {boolean} [checkFuture]
* @returns {boolean}
* @throws if date is not valid
*/
const validateArgDate = (argName, checkFuture) => {
return (argv) => {
const dt = `${argv[argName]}` || '';
if(dt) {
validateDate(dt, `--${argName}`);
if(checkFuture && isFuture(dt)) {
throw new Error(`--${argName} cannot exceed current date`);
}
}
return true;
};
};

/**
* Parse command line and configures options
*
* @param {Boolean} test - used for testing only
* @param {Boolean} [test] - used for testing only
*/
const setOptions = (test) => {
const argv = test || yargs
const argv = test || yargs(process.argv.slice(2))
// @ts-ignore
.options(cmdKeys)
Comment on lines +138 to +139
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

LGTM with a minor correction needed

The updates to the setOptions function are good:

  1. The use of validateArgDate for date checks improves code maintainability.
  2. The folderNames check now correctly handles non-array cases.

However, there's a small issue to address:

There's a typo in one of the validateArgDate calls. The third call should be for 'toDate', not 'fromDate':

- .check(validateArgDate('fromDate', true))
+ .check(validateArgDate('toDate'))

Please correct this to ensure proper validation of the 'toDate' argument.

Also applies to: 164-178

.version(false)
.help(true)
.strict(true)
.check((_argv) => {
// super secret shortcut
if(!_argv.date && !_argv.fromDate && !_argv.toDate && Number.isInteger(_argv._[0])) {
const days = Number(_argv._[0]);
const _date = DateTime
.fromFormat(_argv._[0], options.allowedFormat)
.plus({ days: _argv._[0] })
.format('MM/DD/YY');
.fromFormat(days.toString(), options.allowedFormat)
.plus({ days })
.toFormat('MM/DD/YY');
_argv.date = _date;
Comment on lines +147 to +151
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Incorrect date calculation in shortcut

The changes to the date shortcut calculation don't fully address the previous issue:

  1. Attempting to parse days.toString() as a date will fail because it's a number, not a valid date string.
  2. The final date formatting uses 'MM/DD/YY', which doesn't match the options.allowedFormat.

Here's a suggested fix:

const days = Number(_argv._[0]);
const _date = DateTime
    .now()
    .plus({ days })
    .toFormat(options.allowedFormat);
_argv.date = _date;

This change:

  1. Starts with the current date.
  2. Adds the specified number of days.
  3. Formats the result using the allowed format.

This approach ensures correct date calculation and consistent formatting.

}
Comment on lines +147 to 152
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Logical Error: Incorrect date calculation in the shortcut

The code attempts to parse days.toString() using options.allowedFormat, which is likely to fail since days is a number and may not match the date format. This results in an invalid date parsing.

Proposed fix:

Start with the current date and add days to it:

if(!_argv.date && !_argv.fromDate && !_argv.toDate && Number.isInteger(_argv._[0])) {
    const days = Number(_argv._[0]);
    const _date = DateTime
-       .fromFormat(days.toString(), options.allowedFormat)
+       .now()
        .plus({ days })
-       .toFormat('MM/DD/YY');
+       .toFormat(options.allowedFormat);
    _argv.date = _date;
}

This change initializes the date from the current date and adds the specified number of days, then formats it using the allowed format.

Committable suggestion was skipped due to low confidence.

else {
Expand All @@ -138,53 +161,40 @@ const setOptions = (test) => {
}
return true;
})
.check(({ date }) => {
if(date) {
validateDate(date, '--date');
if(isFuture(date)) {
throw new Error('--date cannot exceed current date');
}
}
return true;
})
.check(({ fromDate }) => {
if(fromDate) {
validateDate(fromDate, '--from-date');
if(isFuture(fromDate)) {
throw new Error('--from-date cannot exceed current date');
}
}
return true;
})
.check(({ toDate }) => {
if(toDate) {
validateDate(toDate, '--to-date');
}
return true;
})
.check(({ folderNames }) => {
.check(validateArgDate('date', true))
.check(validateArgDate('fromDate', true))
.check(validateArgDate('fromDate', true))
.check(validateArgDate('toDate'))
.check((argv) => {
const folderNames = Array.isArray(argv.folderNames) ? argv.folderNames : [];
if(folderNames && !folderNames.length) {
throw new Error('--folder-names requires at least one name');
}
return true;
})
.check(validatePath)
.check(argv => {
const devRoot = argv.devRoot;
return validatePath(devRoot);
})
.argv;

// @ts-ignore
if(argv.date) {
// @ts-ignore
const date = DateTime.fromFormat(argv.date, options.allowedFormat);
module.exports.options.fromDate = date.startOf('day');
module.exports.options.toDate = date.endOf('day');
options.fromDate = date.startOf('day');
options.toDate = date.endOf('day');
}
else {
module.exports.options.fromDate = argv.fromDate ? DateTime.fromFormat(argv.fromDate, options.allowedFormat).startOf('day') : null;
module.exports.options.toDate = argv.toDate ? DateTime.fromFormat(argv.toDate, options.allowedFormat).endOf('day') : null;
// @ts-ignore
options.fromDate = argv.fromDate ? DateTime.fromFormat(argv.fromDate, options.allowedFormat).startOf('day') : null;
// @ts-ignore
options.toDate = argv.toDate ? DateTime.fromFormat(argv.toDate, options.allowedFormat).endOf('day') : null;
}
module.exports.options.devRoot = argv.devRoot;
module.exports.options.folderNames = argv.folderNames || [];
// @ts-ignore
options.devRoot = argv.devRoot;
// @ts-ignore
options.folderNames = argv.folderNames || [];
};

module.exports = {
options
, setOptions
};
export { options, setOptions };
21 changes: 14 additions & 7 deletions src/grefplus/index.js → src/grefplus/index.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,20 @@
/* eslint no-console:off */
require('./cmdline').setOptions();
const { basename } = require('path');
const { allRepoPaths } = require('../common/repos');
const { promisify } = require('util');
const _exec = promisify(require('child_process').exec);
const { DateTime } = require('luxon');
const { options } = require('./cmdline');

import { setOptions, options } from './cmdline.mjs';
import { basename } from 'node:path';
import { promisify } from 'node:util';
import { exec } from 'node:child_process';
const _exec = promisify(exec);

import { DateTime } from 'luxon';

import repos from '../common/repos.js';
const { allRepoPaths } = repos;

const DateLength = 6;

setOptions();

/**
* Creates command string
* @param {String} repo - repository base name
Expand Down
Loading