ronantakizawa/codereview-bench huggingface dataset:
Repository: spotify/backstage
File: plugins/scaffolder-backend/src/actions/index.ts (TypeScript)

Code before:
/*
 * Copyright 2026 The Backstage Authors
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */
import { ActionsRegistryService } from '@backstage/backend-plugin-api/alpha';
import { AuthService, DiscoveryService } from '@backstage/backend-plugin-api';
import { createValidateScaffolderAction } from './createValidateScaffolderAction';

export const createScaffolderActions = (options: {
  actionsRegistry: ActionsRegistryService;
  auth: AuthService;
  discovery: DiscoveryService;
}) => {
  createValidateScaffolderAction(options);
};


Review comment (refactor):
```suggestion
  createDryRunTemplateAction(options);
```

Code after:
/*
 * Copyright 2026 The Backstage Authors
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */
import { ActionsRegistryService } from '@backstage/backend-plugin-api/alpha';
import { ScaffolderService } from '@backstage/plugin-scaffolder-node';
import { createDryRunTemplateAction } from './createDryRunTemplateAction';

export const createScaffolderActions = (options: {
  actionsRegistry: ActionsRegistryService;
  scaffolderService: ScaffolderService;
}) => {
  createDryRunTemplateAction(options);
};
Repository: spotify/backstage
File: plugins/catalog-react/src/alpha/blueprints/extensionData.tsx (TypeScript)

Code before:
/** @internal */
export const entityContentIconDataRef = createExtensionDataRef<
  string | ReactElement
>().with({
  id: 'catalog.entity-content-icon',
});

/** @internal */
export const entityFilterFunctionDataRef = createExtensionDataRef<
  (entity: Entity) => boolean
>().with({ id: 'catalog.entity-filter-function' });

/** @internal */
export const entityFilterExpressionDataRef =
  createExtensionDataRef<string>().with({
    id: 'catalog.entity-filter-expression',
  });

/** @alpha */
export type EntityContentGroupDefinition = {
  title: string;
  icon?: string | ReactElement;
  /** Other group IDs that should be treated as aliases for this group. */
  aliases?: string[];
  /** How to sort the content items within this group. Overrides the page-level default. */
  contentOrder?: 'alpha' | 'natural';
};

/** @alpha */
export type EntityContentGroupDefinitions = Record<
  string,
  EntityContentGroupDefinition
>;

/**
 * @alpha
 * Default entity content groups.
 */
export const defaultEntityContentGroupDefinitions = {
  overview: {
    title: 'Overview',
  },
  documentation: {
    title: 'Documentation',
  },
  development: {
    title: 'Development',
  },
  deployment: {
    title: 'Deployment',
  },

Review comment (suggestion):
Let's make it `'title'` instead of `'alpha'`, in case we want to add ordering by for example ID or something else in the future

Code after:
/** @internal */
export const entityContentIconDataRef = createExtensionDataRef<
  string | ReactElement
>().with({
  id: 'catalog.entity-content-icon',
});

/** @internal */
export const entityFilterFunctionDataRef = createExtensionDataRef<
  (entity: Entity) => boolean
>().with({ id: 'catalog.entity-filter-function' });

/** @internal */
export const entityFilterExpressionDataRef =
  createExtensionDataRef<string>().with({
    id: 'catalog.entity-filter-expression',
  });

/** @alpha */
export type EntityContentGroupDefinitions = Record<
  string,
  {
    title: string;
    icon?: string | ReactElement;
    /** Other group IDs that should be treated as aliases for this group. */
    aliases?: string[];
    /** How to sort the content items within this group. Overrides the page-level default. */
    contentOrder?: 'title' | 'natural';
  }
>;

/**
 * @alpha
 * Default entity content groups.
 */
export const defaultEntityContentGroupDefinitions = {
  overview: {
    title: 'Overview',
  },
  documentation: {
    title: 'Documentation',
  },
  development: {
    title: 'Development',
  },
  deployment: {
    title: 'Deployment',
  },
  operation: {
    title: 'Operation',
  },
Repository: spotify/backstage
File: plugins/catalog-react/src/alpha/blueprints/extensionData.tsx (TypeScript)

Code before:

/** @internal */
export const entityFilterFunctionDataRef = createExtensionDataRef<
  (entity: Entity) => boolean
>().with({ id: 'catalog.entity-filter-function' });

/** @internal */
export const entityFilterExpressionDataRef =
  createExtensionDataRef<string>().with({
    id: 'catalog.entity-filter-expression',
  });

/** @alpha */
export type EntityContentGroupDefinition = {
  title: string;
  icon?: string | ReactElement;
  /** Other group IDs that should be treated as aliases for this group. */
  aliases?: string[];
  /** How to sort the content items within this group. Overrides the page-level default. */
  contentOrder?: 'alpha' | 'natural';
};

/** @alpha */
export type EntityContentGroupDefinitions = Record<
  string,
  EntityContentGroupDefinition
>;

/**
 * @alpha
 * Default entity content groups.
 */
export const defaultEntityContentGroupDefinitions = {
  overview: {
    title: 'Overview',
  },
  documentation: {
    title: 'Documentation',
  },
  development: {
    title: 'Development',
  },
  deployment: {
    title: 'Deployment',
  },
  operation: {
    title: 'Operation',
  },
  observability: {
    title: 'Observability',
  },

Review comment (suggestion):
let's keep the type inlined here instead of declaring `EntityContentGroupDefinition`

Code after:

/** @internal */
export const entityFilterFunctionDataRef = createExtensionDataRef<
  (entity: Entity) => boolean
>().with({ id: 'catalog.entity-filter-function' });

/** @internal */
export const entityFilterExpressionDataRef =
  createExtensionDataRef<string>().with({
    id: 'catalog.entity-filter-expression',
  });

/** @alpha */
export type EntityContentGroupDefinitions = Record<
  string,
  {
    title: string;
    icon?: string | ReactElement;
    /** Other group IDs that should be treated as aliases for this group. */
    aliases?: string[];
    /** How to sort the content items within this group. Overrides the page-level default. */
    contentOrder?: 'title' | 'natural';
  }
>;

/**
 * @alpha
 * Default entity content groups.
 */
export const defaultEntityContentGroupDefinitions = {
  overview: {
    title: 'Overview',
  },
  documentation: {
    title: 'Documentation',
  },
  development: {
    title: 'Development',
  },
  deployment: {
    title: 'Deployment',
  },
  operation: {
    title: 'Operation',
  },
  observability: {
    title: 'Observability',
  },
} satisfies EntityContentGroupDefinitions;

/**
Repository: spotify/luigi
File: test/target_test.py (Python)

Code before:
        except MyException as ex:
            self.fs.rename_dont_move.call_count == 2
            assert ex is orig_ex
        else:
            assert False

    def test_temporary_path_directory(self):
        target_slash = self.target_cls('/tmp/dir/')
        target_noslash = self.target_cls('/tmp/dir')

        with target_slash.temporary_path() as tmp_path:
            assert re.match(r'/tmp/dir-luigi-tmp-\d{10}/', tmp_path)
        self.fs.rename_dont_move.assert_called_once_with(tmp_path, target_slash.path)

        with target_noslash.temporary_path() as tmp_path:
            assert re.match(r'/tmp/dir-luigi-tmp-\d{10}', tmp_path)
        self.fs.rename_dont_move.assert_called_with(tmp_path, target_noslash.path)

    def test_windowsish_dir(self):
        target = self.target_cls(r'''C:\my\folder''' + "\\")
        pattern = r'''C:\\my\\folder-luigi-tmp-\d{10}''' + r"\\"

        with target.temporary_path() as tmp_path:
            assert re.match(pattern, tmp_path)
        self.fs.rename_dont_move.assert_called_once_with(tmp_path, target.path)


    def test_hadoopish_dir(self):
        target = self.target_cls(r'''hdfs:///user/arash/myfile.uids''')

        with target.temporary_path() as tmp_path:
            assert re.match(r'''hdfs:///user/arash/myfile.uids-luigi-tmp-\d{10}''', tmp_path)
        self.fs.rename_dont_move.assert_called_once_with(tmp_path, target.path)


Review comment (suggestion):
To save you the trouble of reading the Travis report, your only issue was a flake8 complaint. You included too may `\n` here.


Code after:
        except MyException as ex:
            self.fs.rename_dont_move.call_count == 2
            assert ex is orig_ex
        else:
            assert False

    def test_temporary_path_directory(self):
        target_slash = self.target_cls('/tmp/dir/')
        target_noslash = self.target_cls('/tmp/dir')

        with target_slash.temporary_path() as tmp_path:
            assert re.match(r'/tmp/dir-luigi-tmp-\d{10}/', tmp_path)
        self.fs.rename_dont_move.assert_called_once_with(tmp_path, target_slash.path)

        with target_noslash.temporary_path() as tmp_path:
            assert re.match(r'/tmp/dir-luigi-tmp-\d{10}', tmp_path)
        self.fs.rename_dont_move.assert_called_with(tmp_path, target_noslash.path)

    def test_windowsish_dir(self):
        target = self.target_cls(r'''C:\my\folder''' + "\\")
        pattern = r'''C:\\my\\folder-luigi-tmp-\d{10}''' + r"\\"

        with target.temporary_path() as tmp_path:
            assert re.match(pattern, tmp_path)
        self.fs.rename_dont_move.assert_called_once_with(tmp_path, target.path)

    def test_hadoopish_dir(self):
        target = self.target_cls(r'''hdfs:///user/arash/myfile.uids''')

        with target.temporary_path() as tmp_path:
            assert re.match(r'''hdfs:///user/arash/myfile.uids-luigi-tmp-\d{10}''', tmp_path)
        self.fs.rename_dont_move.assert_called_once_with(tmp_path, target.path)
Repository: spotify/luigi
File: test/execution_summary_test.py (Python)

Code before:
        self.assertIn('Luigi Execution Summary', s)
        self.assertNotIn('00:00:00', s)
        self.assertNotIn('\n\n\n', s)

    def test_status_with_task_retry(self):
        class Foo(luigi.Task):
            run_count = 0

            def run(self):
                self.run_count += 1
                if self.run_count == 1:
                    raise ValueError()

            def complete(self):
                return self.run_count > 0

        self.run_task(Foo())
        self.run_task(Foo())
        d = self.summary_dict()
        self.assertEqual({Foo()}, d['completed'])
        self.assertEqual({Foo()}, d['ever_failed'])
        self.assertFalse(d['upstream_failure'])
        self.assertFalse(d['upstream_missing_dependency'])
        self.assertFalse(d['run_by_other_worker'])
        self.assertFalse(d['still_pending_ext'])
        s = self.summary()
        self.assertIn('Scheduled 1 task', s)
        self.assertIn('Luigi Execution Summary', s)
        self.assertIn('\n\nThis progress looks :) because there were failed tasks but they all suceeded in a retry', s)


Review comment (bug):
Shouldn't you also test `self.assertFalse(d['failed'])`?

Code after:
        self.assertIn('Luigi Execution Summary', s)
        self.assertNotIn('00:00:00', s)
        self.assertNotIn('\n\n\n', s)

    """
    Test that a task once crashing and then succeeding should be counted as no failure.
    """
    def test_status_with_task_retry(self):
        class Foo(luigi.Task):
            run_count = 0

            def run(self):
                self.run_count += 1
                if self.run_count == 1:
                    raise ValueError()

            def complete(self):
                return self.run_count > 0

        self.run_task(Foo())
        self.run_task(Foo())
        d = self.summary_dict()
        self.assertEqual({Foo()}, d['completed'])
        self.assertEqual({Foo()}, d['ever_failed'])
        self.assertFalse(d['failed'])
        self.assertFalse(d['upstream_failure'])
        self.assertFalse(d['upstream_missing_dependency'])
        self.assertFalse(d['run_by_other_worker'])
        self.assertFalse(d['still_pending_ext'])
        s = self.summary()
        self.assertIn('Scheduled 1 task', s)
        self.assertIn('Luigi Execution Summary', s)
        self.assertNotIn('ever failed', s)
        self.assertIn('\n\nThis progress looks :) because there were failed tasks but they all suceeded in a retry', s)
Repository: spotify/luigi
File: test/retcodes_test.py (Python)

Code before:
                yield MissingDataTask()

        self.run_and_expect('RequiringTask --retcode-task-failed 4 --retcode-missing-data 5', 5)
        self.run_and_expect('RequiringTask --retcode-task-failed 7 --retcode-missing-data 6', 7)

    def test_unknown_reason(self):

        class TaskA(luigi.Task):
            def complete(self):
                return True

        class RequiringTask(luigi.Task):
            def requires(self):
                yield TaskA()

        def new_func(*args, **kwargs):
            return None

        with mock.patch('luigi.scheduler.Scheduler.add_task', new_func):
            self.run_and_expect('RequiringTask', 0)
            self.run_and_expect('RequiringTask --retcode-not-run 5', 5)

    @with_config({'worker': {'keep_alive': 'True'}})
    def test_retry_sucess_task(self):
        class Foo(luigi.Task):
            num = luigi.IntParameter()
            run_count = 0

            def priority(self):
                if self.num == 0:
                    return 100
                else:
                    return 0

            def run(self):
                self.run_count += 1
                if self.num == 0 and self.run_count == 1:
                    raise ValueError()

            def complete(self):
                return self.run_count > 0

        self.run_and_expect('Foo --scheduler-retry-delay=0 --num 0', 0)
        self.run_and_expect('Foo --scheduler-retry-delay=0 --retcode-task-failed 5 --num 0', 0)
        self.run_with_config(dict(task_failed='3'), 'Foo --num 0', 0)


Review comment (bug):
Please add a comment here saying what you're testing

```
"""
Test that a task once crashing and then succeeding counts as no failure.
"""
```

Code after:
                yield MissingDataTask()

        self.run_and_expect('RequiringTask --retcode-task-failed 4 --retcode-missing-data 5', 5)
        self.run_and_expect('RequiringTask --retcode-task-failed 7 --retcode-missing-data 6', 7)

    def test_unknown_reason(self):

        class TaskA(luigi.Task):
            def complete(self):
                return True

        class RequiringTask(luigi.Task):
            def requires(self):
                yield TaskA()

        def new_func(*args, **kwargs):
            return None

        with mock.patch('luigi.scheduler.Scheduler.add_task', new_func):
            self.run_and_expect('RequiringTask', 0)
            self.run_and_expect('RequiringTask --retcode-not-run 5', 5)

    """
    Test that a task once crashing and then succeeding should be counted as no failure.
    """
    def test_retry_sucess_task(self):
        class Foo(luigi.Task):
            run_count = 0

            def run(self):
                self.run_count += 1
                if self.run_count == 1:
                    raise ValueError()

            def complete(self):
                return self.run_count > 0

        self.run_and_expect('Foo --scheduler-retry-delay=0', 0)
        self.run_and_expect('Foo --scheduler-retry-delay=0 --retcode-task-failed=5', 0)
        self.run_with_config(dict(task_failed='3'), 'Foo', 0)
Repository: spotify/luigi
File: luigi/contrib/redshift.py (Python)

Code before:
    """
    This mixin is used to provide the same credential properties
    for AWS to all Redshift tasks. It also provides a helper method
    to generate the credentials string for the task.
    """

    @property
    def aws_access_key_id(self):
        """
        Override to return the key id.
        """
        return self._get_s3_configuration_attribute('aws_access_key_id')

    @property
    def aws_secret_access_key(self):
        """
        Override to return the secret access key.
        """
        return self._get_s3_configuration_attribute('aws_secret_access_key')

    @property
    def aws_account_id(self):
        """
        Override to return the account id.
        """
        return None

    @property
    def aws_arn_role_name(self):
        """
        Override to return the arn role name.
        """
        return None

    @property
    def aws_session_token(self):
        """
        Override to return the session token.
        """
        return None

    def _get_s3_configuration_attribute(self, attribute):
        config = luigi.configuration.get_config()

        try:
            value = config.get('s3', attribute)
        except NoSectionError:
            value = None

        if value is None or value == '':
            value = os.environ.get(attribute.upper(), None)

Review comment (security):
If `aws_access_key_id` and `aws_secret_access_key` auto fill from config, why doesn't `aws_account_id` and `aws_arn_role_name`?

Code after:

    @property
    def configuration_section(self):
        """
        Override to change the configuration section used
        to obtain default credentials.
        """
        return 'redshift'

    @property
    def aws_access_key_id(self):
        """
        Override to return the key id.
        """
        return self._get_configuration_attribute('aws_access_key_id')

    @property
    def aws_secret_access_key(self):
        """
        Override to return the secret access key.
        """
        return self._get_configuration_attribute('aws_secret_access_key')

    @property
    def aws_account_id(self):
        """
        Override to return the account id.
        """
        return self._get_configuration_attribute('aws_account_id')

    @property
    def aws_arn_role_name(self):
        """
        Override to return the arn role name.
        """
        return self._get_configuration_attribute('aws_arn_role_name')

    @property
    def aws_session_token(self):
        """
        Override to return the session token.
        """
        return self._get_configuration_attribute('aws_session_token')

    def _get_configuration_attribute(self, attribute):
        config = luigi.configuration.get_config()

        value = config.get(self.configuration_section, attribute, default=None)

        if not value:
            value = os.environ.get(attribute.upper(), None)
Repository: spotify/luigi
File: luigi/contrib/rdbms.py (Python)

Code before:

    @abc.abstractproperty
    def host(self):
        return None

    @abc.abstractproperty
    def database(self):
        return None

    @abc.abstractproperty
    def user(self):
        return None

    @abc.abstractproperty
    def password(self):
        return None

    @abc.abstractproperty
    def table(self):
        return None

    @abc.abstractproperty
    def query(self):
        return None

    @abc.abstractproperty
    def autocommit(self):
        return False

    @abc.abstractmethod
    def run(self):
        raise NotImplementedError("This method must be overridden")

    @abc.abstractmethod
    def output(self):
        """
        Override with an RDBMS Target (e.g. PostgresTarget or RedshiftTarget) to record execution in a marker table
        """
        raise NotImplementedError("This method must be overridden")

    @property
    def update_id(self):
        """
        Override to create a custom marker table 'update_id' signature for Query subclass task instances
        """
        return self.task_id


Review comment (refactor):
I don't think this needs to be an `abstractproperty`. You shouldn't require the user to override it. Just a plain `@property` should suffice.

Code after:

    @abc.abstractproperty
    def host(self):
        return None

    @abc.abstractproperty
    def database(self):
        return None

    @abc.abstractproperty
    def user(self):
        return None

    @abc.abstractproperty
    def password(self):
        return None

    @abc.abstractproperty
    def table(self):
        return None

    @abc.abstractproperty
    def query(self):
        return None

    @property
    def autocommit(self):
        return False

    @abc.abstractmethod
    def run(self):
        raise NotImplementedError("This method must be overridden")

    @abc.abstractmethod
    def output(self):
        """
        Override with an RDBMS Target (e.g. PostgresTarget or RedshiftTarget) to record execution in a marker table
        """
        raise NotImplementedError("This method must be overridden")

    @property
    def update_id(self):
        """
        Override to create a custom marker table 'update_id' signature for Query subclass task instances
        """
        return self.task_id