ronantakizawa/codereview-bench huggingface dataset:
Repository: cookiecutter/cookiecutter
File: tests/test_default_extensions.py (Python)

Code before:
from pathlib import Path

import freezegun
import pytest

from cookiecutter.main import cookiecutter


@pytest.fixture(autouse=True)
def freeze():
    """Fixture. Make time stating during all tests in this file."""
    freezer = freezegun.freeze_time("2015-12-09 23:33:01")
    freezer.start()
    yield
    freezer.stop()


def test_jinja2_time_extension(tmp_path):
    """Verify Jinja2 time extension work correctly."""
    project_dir = cookiecutter(
        'tests/test-extensions/default/', no_input=True, output_dir=str(tmp_path)
    )
    changelog_file = os.path.join(project_dir, 'HISTORY.rst')
    assert os.path.isfile(changelog_file)

    changelog_lines = Path(changelog_file).open(encoding='utf-8').readlines()

    expected_lines = [
        'History\n',
        '-------\n',
        '\n',
        '0.1.0 (2015-12-09)\n',
        '------------------\n',
        '\n',
        'First release on PyPI.\n',
    ]
    assert expected_lines == changelog_lines


def test_jinja2_slugify_extension(tmp_path):
    """Verify Jinja2 slugify extension work correctly."""
    project_dir = cookiecutter(
        'tests/test-extensions/default/', no_input=True, output_dir=str(tmp_path)
    )

    assert os.path.basename(project_dir) == "it-s-slugified-foobar"


def test_jinja2_uuid_extension(tmp_path):
    """Verify Jinja2 uuid extension work correctly."""
    project_dir = cookiecutter(

Review comment (suggestion):
```suggestion
    with Path(changelog_file).open(encoding='utf-8') as f:
        changelog_lines = f.readlines()
```

Code after:
from pathlib import Path

import freezegun
import pytest

from cookiecutter.main import cookiecutter


@pytest.fixture(autouse=True)
def freeze():
    """Fixture. Make time stating during all tests in this file."""
    freezer = freezegun.freeze_time("2015-12-09 23:33:01")
    freezer.start()
    yield
    freezer.stop()


def test_jinja2_time_extension(tmp_path):
    """Verify Jinja2 time extension work correctly."""
    project_dir = cookiecutter(
        'tests/test-extensions/default/', no_input=True, output_dir=str(tmp_path)
    )
    changelog_file = os.path.join(project_dir, 'HISTORY.rst')
    assert os.path.isfile(changelog_file)

    with Path(changelog_file).open(encoding='utf-8') as f:
        changelog_lines = f.readlines()

    expected_lines = [
        'History\n',
        '-------\n',
        '\n',
        '0.1.0 (2015-12-09)\n',
        '------------------\n',
        '\n',
        'First release on PyPI.\n',
    ]
    assert expected_lines == changelog_lines


def test_jinja2_slugify_extension(tmp_path):
    """Verify Jinja2 slugify extension work correctly."""
    project_dir = cookiecutter(
        'tests/test-extensions/default/', no_input=True, output_dir=str(tmp_path)
    )

    assert os.path.basename(project_dir) == "it-s-slugified-foobar"


def test_jinja2_uuid_extension(tmp_path):
    """Verify Jinja2 uuid extension work correctly."""
Repository: cookiecutter/cookiecutter
File: cookiecutter/utils.py (Python)

Code before:


def force_delete(func, path, exc_info):
    """Error handler for `shutil.rmtree()` equivalent to `rm -rf`.

    Usage: `shutil.rmtree(path, onerror=force_delete)`
    From https://docs.python.org/3/library/shutil.html#rmtree-example
    """
    os.chmod(path, stat.S_IWRITE)
    func(path)


def rmtree(path):
    """Remove a directory and all its contents. Like rm -rf on Unix.

    :param path: A directory path.
    """
    shutil.rmtree(path, onerror=force_delete)


def make_sure_path_exists(path: "os.PathLike[str]") -> None:
    """Ensure that a directory exists.

    :param path: A directory tree path for creation.
    """
    logger.debug('Making sure path exists (Create tree if not exist): %s', path)
    try:
        Path(path).mkdir(parents=True, exist_ok=True)
    except OSError as error:
        raise OSError(f'Unable to create directory at {path}') from error


@contextlib.contextmanager
def work_in(dirname=None):
    """Context manager version of os.chdir.

    When exited, returns to the working directory prior to entering.
    """
    curdir = os.getcwd()
    try:
        if dirname is not None:
            os.chdir(dirname)
        yield
    finally:
        os.chdir(curdir)


def make_executable(script_path):
    """Make `script_path` executable.

    :param script_path: The file to change

Review comment (bug):
```suggestion
    logger.debug('Making sure path exists (creates tree if not exist): %s', path)
```

Code after:


def force_delete(func, path, exc_info):
    """Error handler for `shutil.rmtree()` equivalent to `rm -rf`.

    Usage: `shutil.rmtree(path, onerror=force_delete)`
    From https://docs.python.org/3/library/shutil.html#rmtree-example
    """
    os.chmod(path, stat.S_IWRITE)
    func(path)


def rmtree(path):
    """Remove a directory and all its contents. Like rm -rf on Unix.

    :param path: A directory path.
    """
    shutil.rmtree(path, onerror=force_delete)


def make_sure_path_exists(path: "os.PathLike[str]") -> None:
    """Ensure that a directory exists.

    :param path: A directory tree path for creation.
    """
    logger.debug('Making sure path exists (creates tree if not exist): %s', path)
    try:
        Path(path).mkdir(parents=True, exist_ok=True)
    except OSError as error:
        raise OSError(f'Unable to create directory at {path}') from error


@contextlib.contextmanager
def work_in(dirname=None):
    """Context manager version of os.chdir.

    When exited, returns to the working directory prior to entering.
    """
    curdir = os.getcwd()
    try:
        if dirname is not None:
            os.chdir(dirname)
        yield
    finally:
        os.chdir(curdir)


def make_executable(script_path):
    """Make `script_path` executable.

    :param script_path: The file to change
Repository: cookiecutter/cookiecutter
File: tests/test_get_user_config.py (Python)

Code before:

    def _expanduser(path):
        return path.replace('~', 'Users/bob')

    monkeypatch.setattr('os.path.expanduser', _expanduser)

    config_file = 'tests/test-config/config-expand-user.yaml'

    user_config = config.get_user_config(config_file)
    assert user_config['replay_dir'] == 'Users/bob/replay-files'
    assert user_config['cookiecutters_dir'] == 'Users/bob/templates'


def test_expand_vars_for_directories_in_config(monkeypatch):
    """Validate environment variables expanded in user configs."""
    monkeypatch.setenv('COOKIES', 'Users/bob/cookies')

    config_file = 'tests/test-config/config-expand-vars.yaml'

    user_config = config.get_user_config(config_file)
    assert user_config['replay_dir'] == 'Users/bob/cookies/replay-files'
    assert user_config['cookiecutters_dir'] == 'Users/bob/cookies/templates'


def test_specify_config_values():
    """Validate provided custom config values should be respected ."""
    replay_dir = 'Users/bob/cookies/custom-replay-dir'
    custom_config_updated = {**config.DEFAULT_CONFIG, 'replay_dir': replay_dir}

    user_config = config.get_user_config(default_config={'replay_dir': replay_dir})

    assert user_config == custom_config_updated


Review comment (suggestion):
```suggestion
    """Validate provided custom config values should be respected."""
```

Code after:

    def _expanduser(path):
        return path.replace('~', 'Users/bob')

    monkeypatch.setattr('os.path.expanduser', _expanduser)

    config_file = 'tests/test-config/config-expand-user.yaml'

    user_config = config.get_user_config(config_file)
    assert user_config['replay_dir'] == 'Users/bob/replay-files'
    assert user_config['cookiecutters_dir'] == 'Users/bob/templates'


def test_expand_vars_for_directories_in_config(monkeypatch):
    """Validate environment variables expanded in user configs."""
    monkeypatch.setenv('COOKIES', 'Users/bob/cookies')

    config_file = 'tests/test-config/config-expand-vars.yaml'

    user_config = config.get_user_config(config_file)
    assert user_config['replay_dir'] == 'Users/bob/cookies/replay-files'
    assert user_config['cookiecutters_dir'] == 'Users/bob/cookies/templates'


def test_specify_config_values():
    """Validate provided custom config values should be respected."""
    replay_dir = 'Users/bob/cookies/custom-replay-dir'
    custom_config_updated = {**config.DEFAULT_CONFIG, 'replay_dir': replay_dir}

    user_config = config.get_user_config(default_config={'replay_dir': replay_dir})

    assert user_config == custom_config_updated
Repository: cookiecutter/cookiecutter
File: cookiecutter/prompt.py (Python)

Code before:

    return cookiecutter_dict


def choose_nested_template(context: dict, repo_dir: str, no_input: bool = False) -> str:
    """Prompt user to select the nested template to use.

    :param context: Source for field names and sample values.
    :param repo_dir: Repository directory.
    :param no_input: Do not prompt for user input and use only values from context.
    :returns: Path to the selected template.
    """
    cookiecutter_dict = OrderedDict([])
    env = StrictEnvironment(context=context)
    prefix = ""
    prompts = _extract_prompts(context)
    key = "templates"
    config = context['cookiecutter'].get(key, {})
    if config:
        # Pass
        val = prompt_choice_for_template(key, config, no_input)
        template = config[val]["path"]
    else:
        # Old style
        key = "template"
        config = context['cookiecutter'].get(key)
        val = prompt_choice_for_config(
            cookiecutter_dict, env, key, config, no_input, prompts, prefix
        )
        template = re.search(r'\((.*?)\)', val).group(1)

    repo_dir = Path(repo_dir).resolve()
    template_path = (repo_dir / template).resolve()
    # Return path as string
    return f"{template_path}"


Review comment (bug):
This needs to have a default value, as it does above, to prevent crashing in `prompt_choice_for_config()` under this circumstance:

* `key` is not in `context['cookiecutter']`
* `config` is then `None`
* In `prompt_choice_for_config()`, an attempt is made to iterate over `None`

I recommend adding a test case to demonstrate this issue, and then fix it.

Code after:
    :returns: Path to the selected template.
    """
    cookiecutter_dict = OrderedDict([])
    env = StrictEnvironment(context=context)
    prefix = ""
    prompts = context['cookiecutter'].pop('__prompts__', {})
    key = "templates"
    config = context['cookiecutter'].get(key, {})
    if config:
        # Pass
        val = prompt_choice_for_template(key, config, no_input)
        template = config[val]["path"]
    else:
        # Old style
        key = "template"
        config = context['cookiecutter'].get(key, [])
        val = prompt_choice_for_config(
            cookiecutter_dict, env, key, config, no_input, prompts, prefix
        )
        template = re.search(r'\((.+)\)', val).group(1)

    template = Path(template) if template else None
    if not (template and not template.is_absolute()):
        raise ValueError("Illegal template path")

    repo_dir = Path(repo_dir).resolve()
    template_path = (repo_dir / template).resolve()
    # Return path as string
    return f"{template_path}"
Repository: cookiecutter/cookiecutter
File: cookiecutter/prompt.py (Python)

Code before:
def choose_nested_template(context: dict, repo_dir: str, no_input: bool = False) -> str:
    """Prompt user to select the nested template to use.

    :param context: Source for field names and sample values.
    :param repo_dir: Repository directory.
    :param no_input: Do not prompt for user input and use only values from context.
    :returns: Path to the selected template.
    """
    cookiecutter_dict = OrderedDict([])
    env = StrictEnvironment(context=context)
    prefix = ""
    prompts = _extract_prompts(context)
    key = "templates"
    config = context['cookiecutter'].get(key, {})
    if config:
        # Pass
        val = prompt_choice_for_template(key, config, no_input)
        template = config[val]["path"]
    else:
        # Old style
        key = "template"
        config = context['cookiecutter'].get(key)
        val = prompt_choice_for_config(
            cookiecutter_dict, env, key, config, no_input, prompts, prefix
        )
        template = re.search(r'\((.*?)\)', val).group(1)

    repo_dir = Path(repo_dir).resolve()
    template_path = (repo_dir / template).resolve()
    # Return path as string
    return f"{template_path}"


Review comment (suggestion):
This will match on `"()"` and result in `template_path` being the same value as `repo_dir`. Is this the desired behavior?

It's also possible that a user could specify `"(/)"`, which `repo_dir / template` will dutifully resolve to the root directory. You can check this by doing something like:

```python
template_path = pathlib.Path(template)
if template_path.is_absolute():
    raise ValueError('hang on there, buddy')  # or make it relative
```

It's also possible that this won't match on anything, `re.search()` will return `None`, and `.group(1)` will throw an `AttributeError`.

I recommend throwing strange values at this and confirming that it's not possible to generate paths that might lead to e.g. malicious changes to the filesystem.

Code after:
    prefix = ""
    prompts = context['cookiecutter'].pop('__prompts__', {})
    key = "templates"
    config = context['cookiecutter'].get(key, {})
    if config:
        # Pass
        val = prompt_choice_for_template(key, config, no_input)
        template = config[val]["path"]
    else:
        # Old style
        key = "template"
        config = context['cookiecutter'].get(key, [])
        val = prompt_choice_for_config(
            cookiecutter_dict, env, key, config, no_input, prompts, prefix
        )
        template = re.search(r'\((.+)\)', val).group(1)

    template = Path(template) if template else None
    if not (template and not template.is_absolute()):
        raise ValueError("Illegal template path")

    repo_dir = Path(repo_dir).resolve()
    template_path = (repo_dir / template).resolve()
    # Return path as string
    return f"{template_path}"
Repository: cookiecutter/cookiecutter
File: tests/test_find.py (Python)

Code before:

@pytest.fixture()
def env(context):
    """Fixture return the env generated from context."""
    return create_env_with_context(context)


@pytest.mark.parametrize(
    "repo_name,context,expected",
    [
        ("fake-repo-pre", {}, '{{cookiecutter.repo_name}}'),
        (
            "fake-repo-pre2",
            {
                'cookiecutter': {
                    '_jinja2_env_vars': {
                        'variable_start_string': '{%{',
                        'variable_end_string': '}%}',
                    }
                }
            },
            '{%{cookiecutter.repo_name}%}',
        ),
    ],
)
def test_find_template(repo_name, env, expected):
    """Verify correctness of `find.find_template` path detection."""
    repo_dir = Path('tests', repo_name)

    template = find.find_template(repo_dir, env)

    test_dir = Path(repo_dir, expected)
    assert template == test_dir


Review comment (bug):
Would you add a negative `find()` test? I'd like to see demonstrated behavior when the environment variables and the directory name don't align, like "Jinja vars are `{%{` and `}%}` but the directory is `{{ repo }}`".

I'm not advocating that ``{{ / }}`` is a fallback, just wanting a negative test to demonstrate the expected failure case.

Code after:
    return Path('tests', request.param)


@pytest.fixture()
def env(context):
    """Fixture return the env generated from context."""
    return create_env_with_context(context)


@pytest.mark.parametrize(
    "repo_name,context,error_expectation,expected",
    [
        ("fake-repo-pre", {}, does_not_raise(), '{{cookiecutter.repo_name}}'),
        (
            "fake-repo-pre2",
            {
                'cookiecutter': {
                    '_jinja2_env_vars': {
                        'variable_start_string': '{%{',
                        'variable_end_string': '}%}',
                    }
                }
            },
            does_not_raise(),
            '{%{cookiecutter.repo_name}%}',
        ),
        (
            "fake-repo-pre",
            {
                'cookiecutter': {
                    '_jinja2_env_vars': {
                        'variable_start_string': '{%{',
                        'variable_end_string': '}%}',
                    }
                }
            },
            pytest.raises(NonTemplatedInputDirException),
            None,
        ),
        ("fake-repo-bad", {}, pytest.raises(NonTemplatedInputDirException), None),
    ],
    ids=[
        'template with default jinja strings',
        'template with custom jinja strings',
        'template with custom jinja strings but folder with default jinja strings',
        'template missing folder',
    ],
)
def test_find_template(repo_name, env, error_expectation, expected):
    """Verify correctness of `find.find_template` path detection."""
    repo_dir = Path('tests', repo_name)
Repository: corona-warn-app/cwa-app-android
File: Corona-Warn-App/src/main/java/de/rki/coronawarnapp/util/math/Math.kt (Kotlin)

Code before:
package de.rki.coronawarnapp.util.math

import kotlin.math.roundToInt

/**
 * Round a fraction to a decimal number with the required decimal places
 * @param denominator Power of 10
 */
fun Double.roundToDecimal(denominator: Double = 100.0): Double = (this * denominator).roundToInt() / denominator


Review comment (suggestion):
I think `denominator` should not have a default, because any default always depends on the context.

Second, the current default is technically not correct. It correctly rounds 3.599 to 3.6 (actually 3.60, but 0 is suppressed) but it would round 3.559 to 3.56.

Instead, I would suggest to accept an (unsigned) int as number decimal places and calculate the denominator as 10**decimalPlaces.


Code after:
package de.rki.coronawarnapp.util.math

import kotlin.math.pow
import kotlin.math.roundToLong

/**
 * Round a fraction to a decimal number with the required decimal places
 * @param decimalPlaces [UInt] Number of decimal places ex: 1 -> 10 , 2 -> 10^2
 */
fun Double.roundToDecimal(decimalPlaces: UInt): Double {
    val denominator = 10.0.pow(decimalPlaces.toInt())
    return (this * denominator).roundToLong() / denominator
}
Repository: corona-warn-app/cwa-app-android
File: Corona-Warn-App/src/main/java/de/rki/coronawarnapp/datadonation/analytics/modules/keysubmission/AnalyticsKeySubmissionStorage.kt (Kotlin)

Code before:

    val submittedWithCheckIns = dataStore.dataRecovering.distinctUntilChanged(
        key = booleanPreferencesKey("analytics_key_submission_submittedWithCheckIns$sharedPrefKeySuffix"),
        defaultValue = false
    )

    suspend fun updateSubmittedWithCheckIns(value: Boolean) = dataStore.trySetValue(
        preferencesKey = booleanPreferencesKey(
            "analytics_key_submission_submittedWithCheckIns$sharedPrefKeySuffix"
        ),
        value = value
    )

    suspend fun clear() {
        Timber.tag(TAG).d("clear()")
        val keys = dataStore.data.first().asMap().keys.filter {
            if (sharedPrefKeySuffix.isEmpty()) {
                it.name.endsWith(AnalyticsRAKeySubmissionStorage.sharedPrefKeySuffix).not()
            } else {
                it.name.endsWith(AnalyticsRAKeySubmissionStorage.sharedPrefKeySuffix)
            }
        }
        dataStore.edit { prefs -> keys.forEach { key -> prefs.remove(key) } }
        Timber.d(
            dataStore.data.first().asMap().keys.toString()
        )
    }
}

private val TAG = tag<AnalyticsKeySubmissionStorage>()


Review comment (suggestion):
clear before was clearing all keys regardless of which Test Type storage triggered `clear` and this lead to what ever test is registered last it will clear pervious test collected data not only the relevant test type 

Code after:

    val submittedWithCheckIns = dataStore.dataRecovering.distinctUntilChanged(
        key = booleanPreferencesKey("analytics_key_submission_submittedWithCheckIns$sharedPrefKeySuffix"),
        defaultValue = false
    )

    suspend fun updateSubmittedWithCheckIns(value: Boolean) = dataStore.trySetValue(
        preferencesKey = booleanPreferencesKey(
            "analytics_key_submission_submittedWithCheckIns$sharedPrefKeySuffix"
        ),
        value = value
    )

    suspend fun clear() {
        Timber.tag(TAG).d("clear()")
        val keys = dataStore.data.first().asMap().keys.filter {
            if (sharedPrefKeySuffix.isEmpty()) {
                it.name.endsWith(AnalyticsRAKeySubmissionStorage.sharedPrefKeySuffix).not()
            } else {
                it.name.endsWith(AnalyticsRAKeySubmissionStorage.sharedPrefKeySuffix)
            }
        }
        dataStore.edit { prefs -> keys.forEach { key -> prefs.remove(key) } }
    }
}

private val TAG = tag<AnalyticsKeySubmissionStorage>()
Repository: corona-warn-app/cwa-app-android
File: Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/onboarding/OnboardingPrivacyViewModel.kt (Kotlin)

Code before:
package de.rki.coronawarnapp.ui.onboarding

import androidx.lifecycle.asLiveData
import dagger.assisted.AssistedFactory
import dagger.assisted.AssistedInject
import de.rki.coronawarnapp.eol.AppEol
import de.rki.coronawarnapp.util.coroutine.DefaultDispatcherProvider
import de.rki.coronawarnapp.util.coroutine.DispatcherProvider
import de.rki.coronawarnapp.util.ui.SingleLiveEvent
import de.rki.coronawarnapp.util.viewmodel.CWAViewModel
import de.rki.coronawarnapp.util.viewmodel.SimpleCWAViewModelFactory

class OnboardingPrivacyViewModel @AssistedInject constructor(
    eol: AppEol,
    dispatcherProvider: DispatcherProvider = DefaultDispatcherProvider(),
) : CWAViewModel() {
    val routeToScreen: SingleLiveEvent<OnboardingNavigationEvents> = SingleLiveEvent()
    val isEol = eol.isEol.asLiveData(context = dispatcherProvider.Default)

    fun onNextButtonClick(eolSkip: Boolean) {
        if (eolSkip) {
            routeToScreen.postValue(OnboardingNavigationEvents.NavigateToMainActivity)
        } else {
            routeToScreen.postValue(OnboardingNavigationEvents.NavigateToOnboardingTracing)
        }
    }

    fun onBackButtonClick() {
        routeToScreen.postValue(OnboardingNavigationEvents.NavigateToOnboardingFragment)
    }

    @AssistedFactory
    interface Factory : SimpleCWAViewModelFactory<OnboardingPrivacyViewModel>
}


Review comment (suggestion):
You are exposing `isEol` from the view model to the fragment which keeps track of the is flag and then when a button is clicked you pass the flag back to the view model. why don’t you just check in the viewmodel upon button click ?
```kotlin
fun onNextButtonClick() = launch{
 routeToScreen.postValue( 
    if(eol.isEol.first()) NavigateToMainActivity else NavigateToOnboardingTracing
 )
}
```


Code after:
package de.rki.coronawarnapp.ui.onboarding

import dagger.assisted.AssistedFactory
import dagger.assisted.AssistedInject
import de.rki.coronawarnapp.eol.AppEol
import de.rki.coronawarnapp.util.ui.SingleLiveEvent
import de.rki.coronawarnapp.util.viewmodel.CWAViewModel
import de.rki.coronawarnapp.util.viewmodel.SimpleCWAViewModelFactory
import kotlinx.coroutines.flow.first

class OnboardingPrivacyViewModel @AssistedInject constructor(
    eol: AppEol,
) : CWAViewModel() {
    val routeToScreen: SingleLiveEvent<OnboardingNavigationEvents> = SingleLiveEvent()
    private val isEol = eol.isEol

    fun onNextButtonClick() = launch {
        routeToScreen.postValue(
            if (isEol.first()) {
                OnboardingNavigationEvents.NavigateToMainActivity
            } else {
                OnboardingNavigationEvents.NavigateToOnboardingTracing
            }
        )
    }

    fun onBackButtonClick() {
        routeToScreen.postValue(OnboardingNavigationEvents.NavigateToOnboardingFragment)
    }

    @AssistedFactory
    interface Factory : SimpleCWAViewModelFactory<OnboardingPrivacyViewModel>
}