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

Dmb/circular dependency error when using recommended/49 #59

Merged
Merged
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
34 changes: 18 additions & 16 deletions packages/eslint-plugin-rxjs/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,25 +101,27 @@ type ESLintPlugin = {
configs: Record<string, unknown>;
};

const plugin = {
const basePlugin = {
meta,
rules,
configs: {},
} as ESLintPlugin;
};

const recommendedConfig = {
name: '@smarttools/rxjs/recommended',
plugins: {
'@smarttools/rxjs': basePlugin,
},
rules: {
...recommended.rules,
},
};

if (plugin.configs) {
Object.assign(plugin.configs, {
recommended: {
name: '@smarttools/rxjs/recommended',
plugins: {
'@smarttools/rxjs': plugin,
},
rules: {
...recommended.rules,
},
},
const plugin: ESLintPlugin = {
...basePlugin,
configs: {
recommended: recommendedConfig,
'recommended-legacy': recommended,
});
}
},
};

export = plugin;
1 change: 0 additions & 1 deletion packages/eslint-plugin-rxjs/src/lib/configs/recommended.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import { Linter } from '@typescript-eslint/utils/ts-eslint';

export = {
plugins: ['@smarttools/rxjs'],
name: '@smarttools/rxjs/recommended-legacy',
rules: {
'@smarttools/rxjs/no-async-subscribe': 'error',
'@smarttools/rxjs/no-create': 'error',
Expand Down
8 changes: 0 additions & 8 deletions test/package.json

This file was deleted.

3 changes: 3 additions & 0 deletions tests/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# Tests

The tests in this directory tests all the ways that eslint can be used in combination with this plugin.
14 changes: 14 additions & 0 deletions tests/circular-reference/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
const util = require('util');
const rxjs = require('@smarttools/eslint-plugin-rxjs');

const result =util.inspect(rxjs, { depth: null });

if (result.includes('[Circular')) {
console.log('Circular reference found');
console.log(result);
process.exit(1);
} else {
console.log('No circular reference found');
process.exit(0);
}

46 changes: 46 additions & 0 deletions tests/circular-reference/test.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
#!/bin/bash

# Exit on any error
set -e

# Get the directory where the script is located
SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
ROOT_DIR="$SCRIPT_DIR/../.."

# Define source and destination paths relative to root
SRC_DIR="$ROOT_DIR/dist/packages/eslint-plugin-rxjs"
DEST_DIR="$ROOT_DIR/node_modules/@smarttools/eslint-plugin-rxjs"

# Check if source directory exists
if [ ! -d "$SRC_DIR" ]; then
echo "Error: Source directory $SRC_DIR does not exist"
exit 1
fi

# Create destination parent directory if it doesn't exist
mkdir -p "$(dirname "$DEST_DIR")"

# Remove destination directory if it exists
if [ -d "$DEST_DIR" ]; then
rm -rf "$DEST_DIR"
fi

# Copy the directory
if cp -r "$SRC_DIR" "$DEST_DIR"; then
echo "✓ Successfully copied $SRC_DIR to $DEST_DIR"
else
echo "✗ Failed to copy directory"
exit 1
fi

# Run the index.js file
echo "Running index.js..."
if node "$SCRIPT_DIR/index.js"; then
echo "✓ Tests completed successfully"
exit 0
else
echo "✗ Tests failed"
exit 1
fi
Comment on lines +36 to +44
Copy link

@coderabbitai coderabbitai bot Jan 15, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add cleanup for temporary resources.

The script should clean up resources even if tests fail.

+# Cleanup function
+cleanup() {
+    rm -rf "$DEST_DIR"
+    echo "Cleaned up test resources"
+}
+
+# Set up trap for cleanup
+trap cleanup EXIT
+
 # Run the index.js file
 echo "Running index.js..."
 if node "$SCRIPT_DIR/index.js"; then
     echo "✓ Tests completed successfully"
-    exit 0
+    exit_code=0
 else
     echo "✗ Tests failed"
-    exit 1
+    exit_code=1
 fi
+
+exit $exit_code
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Run the index.js file
echo "Running index.js..."
if node "$SCRIPT_DIR/index.js"; then
echo "✓ Tests completed successfully"
exit 0
else
echo "✗ Tests failed"
exit 1
fi
# Cleanup function
cleanup() {
rm -rf "$DEST_DIR"
echo "Cleaned up test resources"
}
# Set up trap for cleanup
trap cleanup EXIT
# Run the index.js file
echo "Running index.js..."
if node "$SCRIPT_DIR/index.js"; then
echo "✓ Tests completed successfully"
exit_code=0
else
echo "✗ Tests failed"
exit_code=1
fi
exit $exit_code

Copy link
Owner Author

Choose a reason for hiding this comment

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

I've created a followup task for this

Copy link

Choose a reason for hiding this comment

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

@DaveMBush Thanks for creating a follow-up task! That's a great way to ensure the cleanup functionality gets implemented.

⟦ ∙ᴥ∙ ⟧ happy to help



3 changes: 3 additions & 0 deletions tests/circular-reference/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"extends": "../../tsconfig.base.json"
}
41 changes: 41 additions & 0 deletions tests/extends-from/base.eslint.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
module.exports = {
extends: ['plugin:@smarttools/rxjs/recommended-legacy'],
rules: {
'@smarttools/rxjs/finnish': [
'warn',
{
functions: false,
methods: false,
names: {
'^(canActivate|canActivateChild|canDeactivate|canLoad|intercept|resolve|validate|store|_store)$':
false,
},
parameters: false,
properties: false,
strict: false,
types: {
'^EventEmitter$': false,
},
variables: true,
},
],
'@smarttools/rxjs/suffix-subjects': [
'warn',
{
parameters: false,
properties: false,
suffix: '$$',
variables: true,
},
],
'@smarttools/rxjs/no-nested-subscribe': ['warn'],
'@smarttools/rxjs/no-implicit-any-catch': ['off'],
'@smarttools/rxjs/no-unsafe-takeuntil': [
'warn',
{
alias: ['untilDestroyed'],
},
],
'@smarttools/rxjs/no-async-subscribe': 'warn',
},
};
37 changes: 37 additions & 0 deletions tests/extends-from/eslint.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
const { FlatCompat } = require('@eslint/eslintrc');
const js = require('@eslint/js');
const join = require('path').join;
const rxjs = require('@smarttools/eslint-plugin-rxjs');

const compat = new FlatCompat({
baseDirectory: __dirname,
recommendedConfig: js.configs.recommended,
});

module.exports = [
{
ignores: ['**/dist'],
},
{
plugins: {
'@smarttools/rxjs': rxjs,
},
},
...compat
.config({
extends: ['./base.eslint.config.js'],
})
.map((config) => ({
...config,
files: ['**/*.ts'],
languageOptions: {
parser: require('@typescript-eslint/parser'),
parserOptions: {
project: join(__dirname, './tsconfig.json'),
},
},
rules: {
...config.rules,
},
})),
];
3 changes: 3 additions & 0 deletions tests/extends-from/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"type": "commonjs"
}
5 changes: 5 additions & 0 deletions tests/extends-from/sample.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// just a sample to run lint against

import { from } from 'rxjs';

from([1, 2, 3]).subscribe((x) => console.log(x));
42 changes: 42 additions & 0 deletions tests/extends-from/test.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
#!/bin/bash

# Exit on error
set -e

# Get the directory where the script is located
SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
ROOT_DIR="$SCRIPT_DIR/../.."

# Define source and destination paths relative to root
SRC_DIR="$ROOT_DIR/dist/packages/eslint-plugin-rxjs"
DEST_DIR="$ROOT_DIR/node_modules/@smarttools/eslint-plugin-rxjs"

# Check if source directory exists
if [ ! -d "$SRC_DIR" ]; then
echo "Error: Source directory $SRC_DIR does not exist"
exit 1
fi

# Create destination parent directory if it doesn't exist
mkdir -p "$(dirname "$DEST_DIR")"

# Remove destination directory if it exists
if [ -d "$DEST_DIR" ]; then
rm -rf "$DEST_DIR"
fi

# Copy the directory
if cp -r "$SRC_DIR" "$DEST_DIR"; then
echo "✓ Successfully copied $SRC_DIR to $DEST_DIR"
else
echo "✗ Failed to copy directory"
exit 1
fi

# Run ESLint with the specific config and test file
if pnpm eslint --config $SCRIPT_DIR/eslint.config.js $SCRIPT_DIR/sample.ts; then
echo -e "\n✅ Linting PASSED"
else
echo -e "\n❌ Linting FAILED"
exit 1
fi
3 changes: 3 additions & 0 deletions tests/extends-from/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"extends": "../../tsconfig.base.json"
}
53 changes: 53 additions & 0 deletions tests/test.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
#!/bin/bash

# Exit on error
set -e

# Get the directory where the script is located
SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"

# Initialize counters
TOTAL_TESTS=4
PASSED_TESTS=0
FAILED_TESTS=0
FAILED_NAMES=()

# Function to run a test and track its result
function run_test() {
local test_name=$1
local test_path=$2

echo -e "\n📋 Running test: $test_name"
if bash "$test_path"; then
echo "✅ $test_name passed"
PASSED_TESTS=$((PASSED_TESTS + 1))
else
echo "❌ $test_name failed"
FAILED_TESTS=$((FAILED_TESTS + 1))
FAILED_NAMES+=("$test_name")
fi
}
Comment on lines +15 to +29
Copy link

@coderabbitai coderabbitai bot Jan 15, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance test runner with timeout and error handling.

The current implementation could benefit from additional error handling and debugging capabilities.

 function run_test() {
     local test_name=$1
     local test_path=$2
+    local timeout_seconds=300  # 5 minutes timeout
+    local output_file
+    output_file=$(mktemp)
 
     echo -e "\n📋 Running test: $test_name"
-    if bash "$test_path"; then
+    if timeout "$timeout_seconds" bash "$test_path" > "$output_file" 2>&1; then
         echo "✅ $test_name passed"
         PASSED_TESTS=$((PASSED_TESTS + 1))
     else
-        echo "❌ $test_name failed"
+        local exit_code=$?
+        echo "❌ $test_name failed with exit code $exit_code"
+        echo "Last 10 lines of output:"
+        tail -n 10 "$output_file"
         FAILED_TESTS=$((FAILED_TESTS + 1))
         FAILED_NAMES+=("$test_name")
     fi
+    rm -f "$output_file"
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Function to run a test and track its result
function run_test() {
local test_name=$1
local test_path=$2
echo -e "\n📋 Running test: $test_name"
if bash "$test_path"; then
echo "$test_name passed"
PASSED_TESTS=$((PASSED_TESTS + 1))
else
echo "$test_name failed"
FAILED_TESTS=$((FAILED_TESTS + 1))
FAILED_NAMES+=("$test_name")
fi
}
# Function to run a test and track its result
function run_test() {
local test_name=$1
local test_path=$2
local timeout_seconds=300 # 5 minutes timeout
local output_file
output_file=$(mktemp)
echo -e "\n📋 Running test: $test_name"
if timeout "$timeout_seconds" bash "$test_path" > "$output_file" 2>&1; then
echo "$test_name passed"
PASSED_TESTS=$((PASSED_TESTS + 1))
else
local exit_code=$?
echo "$test_name failed with exit code $exit_code"
echo "Last 10 lines of output:"
tail -n 10 "$output_file"
FAILED_TESTS=$((FAILED_TESTS + 1))
FAILED_NAMES+=("$test_name")
fi
rm -f "$output_file"
}

Copy link
Owner Author

Choose a reason for hiding this comment

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

I've created a followup task for this

Copy link

Choose a reason for hiding this comment

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

@DaveMBush Thanks for creating a follow-up task! Let me know if you need any assistance when you get to implementing those enhancements.


If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!


# Run each test
run_test "ESM JS Tests" "$SCRIPT_DIR/esm-js/test.sh"
run_test "Using Recommended Tests" "$SCRIPT_DIR/using-recommended/test.sh"
run_test "Extends From Tests" "$SCRIPT_DIR/extends-from/test.sh"
run_test "Circular Reference Tests" "$SCRIPT_DIR/circular-reference/test.sh"

# Print summary report
echo -e "\n📊 Test Summary Report"
echo "======================="
echo "Total Tests: $TOTAL_TESTS"
echo "Passed: $PASSED_TESTS"
echo "Failed: $FAILED_TESTS"

if [ ${#FAILED_NAMES[@]} -ne 0 ]; then
echo -e "\n❌ Failed Tests:"
for name in "${FAILED_NAMES[@]}"; do
echo "- $name"
done
exit 1
fi

echo -e "\n✨ All tests passed!"
exit 0
21 changes: 21 additions & 0 deletions tests/using-recommended/eslint.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
const rxjs = require('@smarttools/eslint-plugin-rxjs');
const typescriptEslintParser = require('@typescript-eslint/parser');

module.exports = [
rxjs.configs.recommended,
{
files: ['**/*.ts'],
plugins: {
rxjs,
},
},
{
languageOptions: {
parser: typescriptEslintParser,
parserOptions: {
project: './tsconfig.json',
tsconfigRootDir: __dirname,
},
},
},
];
3 changes: 3 additions & 0 deletions tests/using-recommended/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"type": "commonjs"
}
5 changes: 5 additions & 0 deletions tests/using-recommended/sample.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// just a sample to run lint against

import { from } from 'rxjs';

from([1, 2, 3]).subscribe((x) => console.log(x));
42 changes: 42 additions & 0 deletions tests/using-recommended/test.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
#!/bin/bash

# Exit on error
set -e

# Get the directory where the script is located
SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
ROOT_DIR="$SCRIPT_DIR/../.."

# Define source and destination paths relative to root
SRC_DIR="$ROOT_DIR/dist/packages/eslint-plugin-rxjs"
DEST_DIR="$ROOT_DIR/node_modules/@smarttools/eslint-plugin-rxjs"

# Check if source directory exists
if [ ! -d "$SRC_DIR" ]; then
echo "Error: Source directory $SRC_DIR does not exist"
exit 1
fi

# Create destination parent directory if it doesn't exist
mkdir -p "$(dirname "$DEST_DIR")"

# Remove destination directory if it exists
if [ -d "$DEST_DIR" ]; then
rm -rf "$DEST_DIR"
fi

# Copy the directory
if cp -r "$SRC_DIR" "$DEST_DIR"; then
echo "✓ Successfully copied $SRC_DIR to $DEST_DIR"
else
echo "✗ Failed to copy directory"
exit 1
fi

# Run ESLint with the specific config and test file
if pnpm eslint --config $SCRIPT_DIR/eslint.config.js $SCRIPT_DIR/sample.ts; then
echo -e "\n✅ Linting PASSED"
else
echo -e "\n❌ Linting FAILED"
exit 1
fi
Comment on lines +1 to +42
Copy link

@coderabbitai coderabbitai bot Jan 15, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider creating a shared test utilities script.

This script shares significant code with other test scripts. Consider extracting common functionality into a shared utilities script.

Create a new file tests/utils.sh:

#!/bin/bash

setup_test_env() {
    local root_dir="$1"
    local script_dir="$2"
    
    # Create temp directory
    local temp_dir=$(mktemp -d)
    local dest_dir="$temp_dir/node_modules/@smarttools/eslint-plugin-rxjs"
    local src_dir="$root_dir/dist/packages/eslint-plugin-rxjs"
    
    # Copy plugin to test location
    mkdir -p "$(dirname "$dest_dir")"
    cp -r "$src_dir" "$dest_dir" || {
        echo "✗ Failed to copy directory"
        rm -rf "$temp_dir"
        return 1
    }
    
    echo "✓ Successfully copied $src_dir to $dest_dir"
    echo "$temp_dir"
}

cleanup_test_env() {
    local temp_dir="$1"
    rm -rf "$temp_dir"
    echo "Cleaned up test resources"
}

Then modify this script to:

#!/bin/bash
set -e

SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
ROOT_DIR="$SCRIPT_DIR/../.."

# Source common utilities
source "$ROOT_DIR/tests/utils.sh"

# Setup test environment
TEMP_DIR=$(setup_test_env "$ROOT_DIR" "$SCRIPT_DIR")
trap 'cleanup_test_env "$TEMP_DIR"' EXIT

# Run ESLint with the specific config and test file
if pnpm eslint --config "$SCRIPT_DIR/eslint.config.js" "$SCRIPT_DIR/sample.ts"; then
    echo -e "\n✅ Linting PASSED"
    exit 0
else
    echo -e "\n❌ Linting FAILED"
    exit 1
fi

Copy link
Owner Author

Choose a reason for hiding this comment

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

I've created a followup task for this

Copy link

Choose a reason for hiding this comment

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

@DaveMBush Thanks for creating a follow-up task! Let me know if you need any assistance when you get to implementing the shared utilities.


If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!

Loading
Loading