ronantakizawa/codereview-bench huggingface dataset:
Repository: jesseduffield/lazygit
File: pkg/commands/oscommands/cmd_obj_builder.go (Go)

Code before:
package oscommands

import (
	"fmt"
	"os"
	"os/exec"
	"strings"

	"github.com/mgutz/str"
)

type ICmdObjBuilder interface {
	// NewFromArgs takes a slice of strings like []string{"git", "commit"} and returns a new command object.
	New(args []string) ICmdObj
	// NewShell takes a string like `git commit` and returns an executable shell command for it e.g. `sh -c 'git commit'`
	NewShell(commandStr string, aliasesFile string) ICmdObj
	// Quote wraps a string in quotes with any necessary escaping applied. The reason for bundling this up with the other methods in this interface is that we basically always need to make use of this when creating new command objects.
	Quote(str string) string
}

type CmdObjBuilder struct {
	runner   ICmdObjRunner
	platform *Platform
}

// poor man's version of explicitly saying that struct X implements interface Y
var _ ICmdObjBuilder = &CmdObjBuilder{}

func (self *CmdObjBuilder) New(args []string) ICmdObj {
	cmdObj := self.NewWithEnviron(args, os.Environ())
	return cmdObj
}

// A command with explicit environment from env
func (self *CmdObjBuilder) NewWithEnviron(args []string, env []string) ICmdObj {
	cmd := exec.Command(args[0], args[1:]...)
	cmd.Env = env

	return &CmdObj{
		cmd:    cmd,
		runner: self.runner,

Review comment (suggestion):
Should probably update the documentation for this method to explain the purpose of the new variable or add a link back to `Config.md`.

Code after:
package oscommands

import (
	"fmt"
	"os"
	"os/exec"
	"strings"

	"github.com/mgutz/str"
)

type ICmdObjBuilder interface {
	// NewFromArgs takes a slice of strings like []string{"git", "commit"} and returns a new command object.
	New(args []string) ICmdObj
	// NewShell takes a string like `git commit` and returns an executable shell command for it e.g. `sh -c 'git commit'`
	// shellFunctionsFile is an optional file path that will be sourced before executing the command. Callers should pass
	// the value of UserConfig.OS.ShellFunctionsFile.
	NewShell(commandStr string, shellFunctionsFile string) ICmdObj
	// Quote wraps a string in quotes with any necessary escaping applied. The reason for bundling this up with the other methods in this interface is that we basically always need to make use of this when creating new command objects.
	Quote(str string) string
}

type CmdObjBuilder struct {
	runner   ICmdObjRunner
	platform *Platform
}

// poor man's version of explicitly saying that struct X implements interface Y
var _ ICmdObjBuilder = &CmdObjBuilder{}

func (self *CmdObjBuilder) New(args []string) ICmdObj {
	cmdObj := self.NewWithEnviron(args, os.Environ())
	return cmdObj
}

// A command with explicit environment from env
func (self *CmdObjBuilder) NewWithEnviron(args []string, env []string) ICmdObj {
	cmd := exec.Command(args[0], args[1:]...)
	cmd.Env = env

	return &CmdObj{
Repository: jesseduffield/lazygit
File: pkg/commands/git_cmd_obj_builder.go (Go)

Code before:
	innerBuilder *oscommands.CmdObjBuilder
}

var _ oscommands.ICmdObjBuilder = &gitCmdObjBuilder{}

func NewGitCmdObjBuilder(log *logrus.Entry, innerBuilder *oscommands.CmdObjBuilder) *gitCmdObjBuilder {
	// the price of having a convenient interface where we can say .New(...).Run() is that our builder now depends on our runner, so when we want to wrap the default builder/runner in new functionality we need to jump through some hoops. We could avoid the use of a decorator function here by just exporting the runner field on the default builder but that would be misleading because we don't want anybody using that to run commands (i.e. we want there to be a single API used across the codebase)
	updatedBuilder := innerBuilder.CloneWithNewRunner(func(runner oscommands.ICmdObjRunner) oscommands.ICmdObjRunner {
		return &gitCmdObjRunner{
			log:         log,
			innerRunner: runner,
		}
	})

	return &gitCmdObjBuilder{
		innerBuilder: updatedBuilder,
	}
}

var defaultEnvVar = "GIT_OPTIONAL_LOCKS=0"

func (self *gitCmdObjBuilder) New(args []string) oscommands.ICmdObj {
	return self.innerBuilder.New(args).AddEnvVars(defaultEnvVar)
}

func (self *gitCmdObjBuilder) NewShell(cmdStr string, aliasesFile string) oscommands.ICmdObj {
	return self.innerBuilder.NewShell(cmdStr, aliasesFile).AddEnvVars(defaultEnvVar)
}

func (self *gitCmdObjBuilder) Quote(str string) string {
	return self.innerBuilder.Quote(str)
}


Review comment (suggestion):
It might be worthwhile to update the name `aliasesFile` to something like `shFunctionsFile` to bring it inline with what's used in the config.

Code after:
	innerBuilder *oscommands.CmdObjBuilder
}

var _ oscommands.ICmdObjBuilder = &gitCmdObjBuilder{}

func NewGitCmdObjBuilder(log *logrus.Entry, innerBuilder *oscommands.CmdObjBuilder) *gitCmdObjBuilder {
	// the price of having a convenient interface where we can say .New(...).Run() is that our builder now depends on our runner, so when we want to wrap the default builder/runner in new functionality we need to jump through some hoops. We could avoid the use of a decorator function here by just exporting the runner field on the default builder but that would be misleading because we don't want anybody using that to run commands (i.e. we want there to be a single API used across the codebase)
	updatedBuilder := innerBuilder.CloneWithNewRunner(func(runner oscommands.ICmdObjRunner) oscommands.ICmdObjRunner {
		return &gitCmdObjRunner{
			log:         log,
			innerRunner: runner,
		}
	})

	return &gitCmdObjBuilder{
		innerBuilder: updatedBuilder,
	}
}

var defaultEnvVar = "GIT_OPTIONAL_LOCKS=0"

func (self *gitCmdObjBuilder) New(args []string) oscommands.ICmdObj {
	return self.innerBuilder.New(args).AddEnvVars(defaultEnvVar)
}

func (self *gitCmdObjBuilder) NewShell(cmdStr string, shellFunctionsFile string) oscommands.ICmdObj {
	return self.innerBuilder.NewShell(cmdStr, shellFunctionsFile).AddEnvVars(defaultEnvVar)
}

func (self *gitCmdObjBuilder) Quote(str string) string {
	return self.innerBuilder.Quote(str)
}
Repository: jesseduffield/lazygit
File: pkg/integration/tests/custom_commands/selected_submodule.go (Go)

Code before:
		shell.CloneIntoSubmodule("submodule", "path/submodule")
		shell.Commit("Add submodule")
	},
	SetupConfig: func(cfg *config.AppConfig) {
		cfg.GetUserConfig().CustomCommands = []config.CustomCommand{
			{
				Key:     "X",
				Context: "submodules",
				Command: "printf '%s' '{{ .SelectedSubmodule.Path }}' > file.txt",
			},
			{
				Key:     "U",
				Context: "submodules",
				Command: "printf '%s' '{{ .SelectedSubmodule.Url }}' > file.txt",
			},
			{
				Key:     "N",
				Context: "submodules",
				Command: "printf '%s' '{{ .SelectedSubmodule.Name }}' > file.txt",
			},
		}
	},
	Run: func(t *TestDriver, keys config.KeybindingConfig) {
		t.Views().Submodules().
			Focus().
			NavigateToLine(Contains("submodule"))

		t.Views().Submodules().Press("X")
		t.FileSystem().FileContent("file.txt", Equals("path/submodule"))

		t.Views().Submodules().Press("U")
		t.FileSystem().FileContent("file.txt", Equals("../path/submodule"))

		t.Views().Submodules().Press("V")
		t.FileSystem().FileContent("file.txt", Equals("submodule"))
	},
})


Review comment (suggestion):
That's the only line we have in that view, no need to navigate to it. We usually do
```go
			Lines(
				Contains("submodule").IsSelected(),
			)
```
in cases like this.

Code after:
		shell.CloneIntoSubmodule("submodule", "path/submodule")
		shell.Commit("Add submodule")
	},
	SetupConfig: func(cfg *config.AppConfig) {
		cfg.GetUserConfig().CustomCommands = []config.CustomCommand{
			{
				Key:     "X",
				Context: "submodules",
				Command: "printf '%s' '{{ .SelectedSubmodule.Path }}' > file.txt",
			},
			{
				Key:     "U",
				Context: "submodules",
				Command: "printf '%s' '{{ .SelectedSubmodule.Url }}' > file.txt",
			},
			{
				Key:     "N",
				Context: "submodules",
				Command: "printf '%s' '{{ .SelectedSubmodule.Name }}' > file.txt",
			},
		}
	},
	Run: func(t *TestDriver, keys config.KeybindingConfig) {
		t.Views().Submodules().
			Focus().
			Lines(
				Contains("submodule").IsSelected(),
			)

		t.Views().Submodules().Press("X")
		t.FileSystem().FileContent("file.txt", Equals("path/submodule"))

		t.Views().Submodules().Press("U")
		t.FileSystem().FileContent("file.txt", Equals("../submodule"))

		t.Views().Submodules().Press("N")
		t.FileSystem().FileContent("file.txt", Equals("submodule"))
	},
})
Repository: jesseduffield/lazygit
File: pkg/gui/presentation/submodules.go (Go)

Code before:
package presentation

import (
	"strings"

	"github.com/jesseduffield/lazygit/pkg/commands/models"
	"github.com/jesseduffield/lazygit/pkg/theme"
	"github.com/samber/lo"
)

func GetSubmoduleListDisplayStrings(submodules []*models.SubmoduleConfig) [][]string {
	return lo.Map(submodules, func(submodule *models.SubmoduleConfig, _ int) []string {
		return getSubmoduleDisplayStrings(submodule)
	})
}

func getSubmoduleDisplayStrings(s *models.SubmoduleConfig) []string {
	name := s.Name
	if s.ParentModule != nil {
		var indentation strings.Builder
		for p := s.ParentModule; p != nil; p = p.ParentModule {
			indentation.WriteString("  ")
		}

		name = indentation.String() + "- " + s.Name
	}

	return []string{theme.DefaultTextColor.Sprint(name)}
}


Review comment (suggestion):
For this one, I'd rather suggest changing it to something like this:
```go
	if s.ParentModule != nil {
		count := 0
		for p := s.ParentModule; p != nil; p = p.ParentModule {
			count++
		}
		indentation := strings.Repeat("  ", count)
		name = indentation + "- " + s.Name
	}
```

Code after:
package presentation

import (
	"strings"

	"github.com/jesseduffield/lazygit/pkg/commands/models"
	"github.com/jesseduffield/lazygit/pkg/theme"
	"github.com/samber/lo"
)

func GetSubmoduleListDisplayStrings(submodules []*models.SubmoduleConfig) [][]string {
	return lo.Map(submodules, func(submodule *models.SubmoduleConfig, _ int) []string {
		return getSubmoduleDisplayStrings(submodule)
	})
}

func getSubmoduleDisplayStrings(s *models.SubmoduleConfig) []string {
	name := s.Name
	if s.ParentModule != nil {
		count := 0
		for p := s.ParentModule; p != nil; p = p.ParentModule {
			count++
		}
		indentation := strings.Repeat("  ", count)
		name = indentation + "- " + s.Name
	}

	return []string{theme.DefaultTextColor.Sprint(name)}
}
Repository: jina-ai/jina
File: cli/__init__.py (Python)

Code before:
import os
import shutil
import subprocess
import sys

from packaging.version import Version, parse

KNOWN_PLUGINS_URL = (
    'https://raw.githubusercontent.com/jina-ai/jina/master/cli/known_plugins.json'
)


def _get_run_args(print_args: bool = True):
    from jina.helper import get_rich_console
    from jina.parsers import get_main_parser

    console = get_rich_console()

    silent_print = {'help', 'hub'}

    parser = get_main_parser()
    if len(sys.argv) > 1:
        from argparse import _StoreAction, _StoreTrueAction

        from rich import box
        from rich.table import Table

        args, unknown = parser.parse_known_args()

        if unknown:
            from jina.helper import warn_unknown_args

            unknown = list(filter(lambda x: x.startswith('--'), unknown))
            warn_unknown_args(unknown)


Review comment (suggestion):
I believe we could make this accessible through the `https://api.jina.ai/` API instead, but I don't know how to set that up

Code after:
import os
import shutil
import subprocess
import sys

from packaging.version import Version, parse


def _get_run_args(print_args: bool = True):
    from jina.helper import get_rich_console
    from jina.parsers import get_main_parser

    console = get_rich_console()

    silent_print = {'help', 'hub'}

    parser = get_main_parser()
    if len(sys.argv) > 1:
        from argparse import _StoreAction, _StoreTrueAction

        from rich import box
        from rich.table import Table

        args, unknown = parser.parse_known_args()

        if unknown:
            from jina.helper import warn_unknown_args

            unknown = list(filter(lambda x: x.startswith('--'), unknown))
            warn_unknown_args(unknown)

        if args.cli not in silent_print and print_args:
            from jina import __resources_path__

            p = parser._actions[-1].choices[sys.argv[1]]
Repository: jina-ai/jina
File: jina/helper.py (Python)

Code before:
    return _hostname, port, scheme, tls


def _parse_url(host):
    if '://' in host:
        scheme, host = host.split('://')
    else:
        scheme = None

    if ':' in host:
        host, port = host.split(':')
    else:
        port = None

    return scheme, host, port


def is_port_free(host, port):
    with socket(AF_INET, SOCK_STREAM) as session:
        if session.connect_ex((host, port)) == 0:
            return False
        else:
            return True


def _parse_port(port: str) -> Union[int, List]:
    """Parse port

    EXAMPLE USAGE

    .. code-block:: python


        _parse_port('8000')
        8000

        _parse_port('8001:8002:8005')
        [80001, 8002, 8005]

    :param port: the string to parse
    :return: the port or the iterable ports
    """
    try:
        port = int(port)
    except ValueError as e:
        if ',' in port:
            port = [int(port_) for port_ in port.split(',')]
        else:
            raise e
    return port


Review comment (suggestion):
```suggestion
def _parse_ports(port: str) -> Union[int, List]:
```
i think this should be plural

Code after:
    return _hostname, port, scheme, tls


def _parse_url(host):
    if '://' in host:
        scheme, host = host.split('://')
    else:
        scheme = None

    if ':' in host:
        host, port = host.split(':')
    else:
        port = None

    return scheme, host, port


def is_port_free(host, port):
    with socket(AF_INET, SOCK_STREAM) as session:
        if session.connect_ex((host, port)) == 0:
            return False
        else:
            return True


def _parse_ports(port: str) -> Union[int, List]:
    """Parse port

    EXAMPLE USAGE

    .. code-block:: python


        _parse_port('8000')
        8000

        _parse_port('8001:8002:8005')
        [80001, 8002, 8005]

    :param port: the string to parse
    :return: the port or the iterable ports
    """
    try:
        port = int(port)
    except ValueError as e:
        if ',' in port:
            port = [int(port_) for port_ in port.split(',')]
        else:
            raise e
    return port
Repository: jina-ai/jina
File: jina/serve/stream/helper.py (Python)

Code before:
from typing import Iterator, AsyncIterator, Union

from jina.helper import get_or_reuse_loop


class AsyncRequestsIterator:
    """Iterator to allow async iteration of blocking/non-blocking iterator from the Client"""

    def __init__(self, iterator: Union[Iterator, AsyncIterator]) -> None:
        """Async request iterator
        TODO (Deepankar): accept `num_req`

        :param iterator: request iterator
        """
        self.iterator = iterator

    def iterator__next__(self):
        """
        Executed inside a `ThreadPoolExecutor` via `loop.run_in_executor` to avoid following exception.
        "StopIteration interacts badly with generators and cannot be raised into a Future"

        :return: next request or None
        """
        try:
            return self.iterator.__next__()
        except StopIteration:
            return None

    def __aiter__(self):
        return self

    async def __anext__(self):

Review comment (refactor):
I am wondering if we should not name it `_RequestsCounter`. I remember that in the past we have been locked into not refactoring helper function (rename them or move them) because they were not private and thus users could rely on it. If it is a private Class it kind of gave the right to refactor as we want. 

Code after:
import asyncio
from typing import AsyncIterator, Iterator, Optional, Union

from jina.helper import get_or_reuse_loop


class RequestsCounter:
    """Class used to wrap a count integer so that it can be updated inside methods.

    .. code-block:: python

        def count_increment(i: int, rc: RequestCounter):
            i += 1
            rc.count += 1


        c_int = 0
        c_rc = RequestsCounter()
        count_increment(c_int, c_rc)

        assert c_int == 0
        assert c_rc.count == 1
    """

    count = 0


class AsyncRequestsIterator:
    """Iterator to allow async iteration of blocking/non-blocking iterator from the Client"""

    def __init__(
        self,
Repository: jina-ai/jina
File: tests/unit/hubble/test_hubio.py (Python)

Code before:

    monkeypatch.setattr(requests, 'post', _mock_post)

    HubIO.fetch_meta('dummy_mwu_encoder', tag=None, force=True)

    assert mock.call_count == 1

    _, kwargs = mock.call_args_list[0]

    assert kwargs['headers'].get('Authorization') == f'token {auth_token}'


class DownloadMockResponse:
    def __init__(self, response_code: int = 200):
        self.response_code = response_code

    def iter_content(self, buffer=32 * 1024):
        zip_file = Path(__file__).parent / 'dummy_executor.zip'
        with zip_file.open('rb') as f:
            yield f.read(buffer)

    @property
    def status_code(self):
        return self.response_code

@pytest.mark.parametrize('executer_name', ['alias_dummy',None])
def test_pull(test_envs, mocker, monkeypatch, executer_name):
    mock = mocker.Mock()

    def _mock_fetch(
        name,
        tag=None,
        secret=None,
        image_required=True,
        rebuild_image=True,
        force=False,
    ):
        mock(name=name)
        return (
            HubExecutor(
                uuid='dummy_mwu_encoder',
                name=executer_name,
                tag='v0',
                image_name='jinahub/pod.dummy_mwu_encoder',
                md5sum=None,
                visibility=True,
                archive_url=None,
            ),
            False,
        )


Review comment (suggestion):
```suggestion
@pytest.mark.parametrize('executer_name', ['alias_dummy', None])
```

Code after:

    monkeypatch.setattr(requests, 'post', _mock_post)

    HubIO.fetch_meta('dummy_mwu_encoder', tag=None, force=True)

    assert mock.call_count == 1

    _, kwargs = mock.call_args_list[0]

    assert kwargs['headers'].get('Authorization') == f'token {auth_token}'


class DownloadMockResponse:
    def __init__(self, response_code: int = 200):
        self.response_code = response_code

    def iter_content(self, buffer=32 * 1024):
        zip_file = Path(__file__).parent / 'dummy_executor.zip'
        with zip_file.open('rb') as f:
            yield f.read(buffer)

    @property
    def status_code(self):
        return self.response_code

@pytest.mark.parametrize('executor_name', ['alias_dummy', None])
def test_pull(test_envs, mocker, monkeypatch, executor_name):
    mock = mocker.Mock()

    def _mock_fetch(
        name,
        tag=None,
        secret=None,
        image_required=True,
        rebuild_image=True,
        force=False,
    ):
        mock(name=name)
        return (
            HubExecutor(
                uuid='dummy_mwu_encoder',
                name=executor_name,
                tag='v0',
                image_name='jinahub/pod.dummy_mwu_encoder',
                md5sum=None,
                visibility=True,
                archive_url=None,
            ),
            False,
        )
Repository: jina-ai/jina
File: jina/parsers/hubble/push.py (Python)

Code before:
        '-t',
        '--tag',
        action='append',
        help='''
A list of tags. One can use it to distinguish architecture (e.g. `cpu`, `gpu`) or versions (e.g. `v1`, `v2`).

One can later fetch a tagged Executor via `jinahub[+docker]://MyExecutor/gpu`
''',
    )

    gp.add_argument(
        '--protected-tag',
        action='append',
        help='A list of protected tags. Like tag but protected against updates after first push.',
    )

    gp.add_argument(
        '--force-update',
        '--force',
        type=str,
        help='If set, push will overwrite the Executor on the Hub that shares the same NAME or UUID8 identifier',
    )
    gp.add_argument(
        '--build-env',
        type=str,
        help='A list of environment variables. It will be used in project build phrase.',
    )
    gp.add_argument(
        '--secret',
        type=str,
        help='The secret for overwrite a Hub executor',
    )

    gp.add_argument(
        '--no-cache',
        action='store_true',
        default=False,
        help='If set, "--no-cache" option will be added to the Docker build.',
    )

    gp = add_arg_group(parser, title='Visibility')

    mutually_exclusive_group = gp.add_mutually_exclusive_group()

    mutually_exclusive_group.add_argument(
        '--public',
        action='store_true',
        default=argparse.SUPPRESS,
        help='If set, the pushed executor is visible to public',
    )


Review comment (suggestion):
```suggestion
        help='A list of environment variables. It will be used in project build phase.',
```

Code after:
        '-t',
        '--tag',
        action='append',
        help='''
A list of tags. One can use it to distinguish architecture (e.g. `cpu`, `gpu`) or versions (e.g. `v1`, `v2`).

One can later fetch a tagged Executor via `jinahub[+docker]://MyExecutor/gpu`
''',
    )

    gp.add_argument(
        '--protected-tag',
        action='append',
        help='A list of protected tags. Like tag but protected against updates after first push.',
    )

    gp.add_argument(
        '--force-update',
        '--force',
        type=str,
        help='If set, push will overwrite the Executor on the Hub that shares the same NAME or UUID8 identifier',
    )
    gp.add_argument(
        '--build-env',
        type=str,
        help='A list of environment variables. It will be used in project build phase.',
    )
    gp.add_argument(
        '--secret',
        type=str,
        help='The secret for overwrite a Hub executor',
    )

    gp.add_argument(
        '--no-cache',
        action='store_true',
        default=False,
        help='If set, "--no-cache" option will be added to the Docker build.',
    )

    gp = add_arg_group(parser, title='Visibility')

    mutually_exclusive_group = gp.add_mutually_exclusive_group()

    mutually_exclusive_group.add_argument(
        '--public',
        action='store_true',
        default=argparse.SUPPRESS,
        help='If set, the pushed executor is visible to public',
    )