ronantakizawa/codereview-bench huggingface dataset:
Repository: cli/cli
File: api/queries_repo.go (Go)

Code before:
	if err := g.Wait(); err != nil {
		return nil, err
	}

	return &result, nil
}

type RepoResolveInput struct {
	Assignees  []string
	Reviewers  []string
	Labels     []string
	Projects   []string
	Milestones []string
}

// RepoResolveMetadataIDs looks up GraphQL node IDs in bulk
func RepoResolveMetadataIDs(client *Client, repo ghrepo.Interface, input RepoResolveInput) (*RepoMetadataResult, error) {
	users := input.Assignees
	hasUser := func(target string) bool {
		for _, u := range users {
			if strings.EqualFold(u, target) {
				return true
			}
		}
		return false
	}

	var teams []string
	for _, r := range input.Reviewers {
		if i := strings.IndexRune(r, '/'); i > -1 {
			teams = append(teams, r[i+1:])
		} else if !hasUser(r) {
			users = append(users, r)
		}
	}

	// there is no way to look up projects nor milestones by name, so preload them all
	mi := RepoMetadataInput{
		Projects:   len(input.Projects) > 0,
		Milestones: len(input.Milestones) > 0,
	}
	result, err := RepoMetadata(client, repo, mi)
	if err != nil {
		return result, err
	}
	if len(users) == 0 && len(teams) == 0 && len(input.Labels) == 0 {
		return result, nil
	}

	query := &bytes.Buffer{}
	fmt.Fprint(query, "query RepositoryResolveMetadataIDs {\n")

Review comment (suggestion):
So, in my opinion, the previous approach of "accepting aggregate projects" and "returning v1 and v2 separately" signified something funny. The result was that we'd have to prop drill the support variable everywhere like so: https://github.com/cli/cli/compare/trunk...cli-9430#diff-5c07fcc22c80bad9356b4c3e1c794101855bcac70b031572c2084dc6d2023abaR872

I think it makes a lot more sense to make the decision as high up the stack as we can and then use types to correctly model expecations.

Not also that this changed to a `bool` because the values were never used.

Code after:
		})
	}

	if input.Milestones {
		g.Go(func() error {
			milestones, err := RepoMilestones(client, repo, "open")
			if err != nil {
				err = fmt.Errorf("error fetching milestones: %w", err)
			}
			result.Milestones = milestones
			return err
		})
	}

	if err := g.Wait(); err != nil {
		return nil, err
	}

	return &result, nil
}

type RepoResolveInput struct {
	Assignees  []string
	Reviewers  []string
	Labels     []string
	ProjectsV1 bool
	ProjectsV2 bool
	Milestones []string
}

// RepoResolveMetadataIDs looks up GraphQL node IDs in bulk
func RepoResolveMetadataIDs(client *Client, repo ghrepo.Interface, input RepoResolveInput) (*RepoMetadataResult, error) {
	users := input.Assignees
	hasUser := func(target string) bool {
		for _, u := range users {
			if strings.EqualFold(u, target) {
				return true
			}
		}
		return false
	}

	var teams []string
	for _, r := range input.Reviewers {
		if i := strings.IndexRune(r, '/'); i > -1 {
			teams = append(teams, r[i+1:])
		} else if !hasUser(r) {
			users = append(users, r)
		}
	}
Repository: cli/cli
File: api/queries_repo.go (Go)

Code before:
			if err != nil {
				err = fmt.Errorf("error fetching current login: %w", err)
			}
			result.CurrentLogin = login
			return err
		})
	}
	if input.Labels {
		g.Go(func() error {
			labels, err := RepoLabels(client, repo)
			if err != nil {
				err = fmt.Errorf("error fetching labels: %w", err)
			}
			result.Labels = labels
			return err
		})
	}
	if input.Projects {
		g.Go(func() error {
			var err error
			result.Projects, result.ProjectsV2, err = relevantProjects(client, repo)
			return err
		})
	}
	if input.Milestones {
		g.Go(func() error {
			milestones, err := RepoMilestones(client, repo, "open")
			if err != nil {
				err = fmt.Errorf("error fetching milestones: %w", err)
			}
			result.Milestones = milestones
			return err
		})
	}

	if err := g.Wait(); err != nil {
		return nil, err
	}

	return &result, nil
}

type RepoResolveInput struct {
	Assignees  []string
	Reviewers  []string
	Labels     []string
	Projects   []string
	Milestones []string
}

// RepoResolveMetadataIDs looks up GraphQL node IDs in bulk

Review comment (suggestion):
Shouldn't this field (i.e., `RepoMetadataResult.Projects`) be renamed to `ProjectsV1`? I know this might affect multiple places, but it will surely makes things less confusing.

Code after:
	if input.Reviewers {
		g.Go(func() error {
			login, err := CurrentLoginName(client, repo.RepoHost())
			if err != nil {
				err = fmt.Errorf("error fetching current login: %w", err)
			}
			result.CurrentLogin = login
			return err
		})
	}

	if input.Labels {
		g.Go(func() error {
			labels, err := RepoLabels(client, repo)
			if err != nil {
				err = fmt.Errorf("error fetching labels: %w", err)
			}
			result.Labels = labels
			return err
		})
	}

	if input.ProjectsV1 {
		g.Go(func() error {
			var err error
			result.Projects, err = v1Projects(client, repo)
			return err
		})
	}

	if input.ProjectsV2 {
		g.Go(func() error {
			var err error
			result.ProjectsV2, err = v2Projects(client, repo)
			return err
		})
	}

	if input.Milestones {
		g.Go(func() error {
			milestones, err := RepoMilestones(client, repo, "open")
			if err != nil {
				err = fmt.Errorf("error fetching milestones: %w", err)
			}
			result.Milestones = milestones
			return err
		})
	}

	if err := g.Wait(); err != nil {
		return nil, err
Repository: cli/cli
File: pkg/cmd/accessibility/accessibility.go (Go)

Code before:
	"github.com/cli/cli/v2/internal/text"
	"github.com/cli/cli/v2/pkg/cmdutil"
	"github.com/cli/cli/v2/pkg/iostreams"
	"github.com/spf13/cobra"
)

const (
	feedbackURL = "https://accessibility.github.com/feedback"
)

type AccessibilityOptions struct {
	IO      *iostreams.IOStreams
	Browser browser.Browser
	Web     bool
}

func NewCmdAccessibility(f *cmdutil.Factory) *cobra.Command {
	opts := AccessibilityOptions{
		IO:      f.IOStreams,
		Browser: f.Browser,
	}

	cmd := &cobra.Command{
		Use:     "accessibility",
		Aliases: []string{"a11y"},
		Short:   "Learn about GitHub CLI accessibility experiences",
		Long:    longDescription(opts.IO),
		Hidden:  true,
		RunE: func(cmd *cobra.Command, args []string) error {
			if opts.Web {
				if opts.IO.IsStdoutTTY() {
					fmt.Fprintf(opts.IO.ErrOut, "Opening %s in your browser.\n", text.DisplayURL(feedbackURL))
				}
				return opts.Browser.Browse(feedbackURL)
			}

			return cmd.Help()
		},
		Example: heredoc.Doc(`
			# Open the GitHub Accessibility site in your browser
			$ gh accessibility --web

			# Display color using customizable, 4-bit accessible colors
			$ gh config set accessible_colors enabled

			# Display issue and pull request labels using RGB hex color codes in terminals that support 24-bit true color
			$ gh config set color_labels enabled

			# Use input prompts without redrawing the screen
			$ gh config set accessible_prompter enabled


Review comment (suggestion):
```suggestion
		Short:   "Learn about GitHub CLI's accessibility experiences",
```

Code after:
	"github.com/cli/cli/v2/internal/text"
	"github.com/cli/cli/v2/pkg/cmdutil"
	"github.com/cli/cli/v2/pkg/iostreams"
	"github.com/spf13/cobra"
)

const (
	webURL = "https://accessibility.github.com/conformance/cli/"
)

type AccessibilityOptions struct {
	IO      *iostreams.IOStreams
	Browser browser.Browser
	Web     bool
}

func NewCmdAccessibility(f *cmdutil.Factory) *cobra.Command {
	opts := AccessibilityOptions{
		IO:      f.IOStreams,
		Browser: f.Browser,
	}

	cmd := &cobra.Command{
		Use:     "accessibility",
		Aliases: []string{"a11y"},
		Short:   "Learn about GitHub CLI's accessibility experiences",
		Long:    longDescription(opts.IO),
		Hidden:  true,
		RunE: func(cmd *cobra.Command, args []string) error {
			if opts.Web {
				if opts.IO.IsStdoutTTY() {
					fmt.Fprintf(opts.IO.ErrOut, "Opening %s in your browser.\n", text.DisplayURL(webURL))
				}
				return opts.Browser.Browse(webURL)
			}

			return cmd.Help()
		},
		Example: heredoc.Doc(`
			# Open the GitHub Accessibility site in your browser
			$ gh accessibility --web

			# Display color using customizable, 4-bit accessible colors
			$ gh config set accessible_colors enabled

			# Use input prompts without redrawing the screen
			$ gh config set accessible_prompter enabled

			# Disable motion-based spinners for progress indicators in favor of text
			$ gh config set spinner disabled
		`),
Repository: cli/cli
File: pkg/cmd/accessibility/accessibility.go (Go)

Code before:
		   - Run %[1]sgh config set color_labels enabled%[1]s
		   - Set %[1]sGH_COLOR_LABELS=enabled%[1]s environment variable

		%[4]s

		Interactive text user interfaces manipulate the terminal cursor to redraw parts
		of the screen, which can be difficult for speech synthesizers or braille displays
		to accurately detect and read.

		To create a more accessible experience, the GitHub CLI gives users the ability to
		disable this interactivity while providing a similar experience using
		non-interactive prompts for user input.

		To enable this experience, use one of the following methods:
		- Run %[1]sgh config set accessible_prompter enabled%[1]s
		- Set %[1]sGH_ACCESSIBLE_PROMPTER=enabled%[1]s environment variable

		%[5]s

		Motion-based spinners communicate in-progress activity by manipulating the
		terminal cursor to create a spinning effect, which can be difficult for users
		with motion sensitivity or miscommunicate information to speech synthesizers.

		To create a more accessible experience, the GitHub CLI gives users the ability to
		disable this interactivity while providing a similar experience using text-based
		progress indicators.

		To enable this experience, use one of the following methods:
		- Run %[1]sgh config set spinner disabled%[1]s
		- Set %[1]sGH_SPINNER_DISABLED=yes%[1]s environment variable

		%[6]s

		We invite you to join us in improving GitHub CLI accessibility by sharing your
		feedback and ideas through GitHub Accessibility feedback channels:

		%[7]s
	`, "`", title, color, prompter, spinner, feedback, feedbackURL)
}


Review comment (suggestion):
```suggestion
		For a more accessible experience, this interactivity can be disabled in favor 
		of text-based progress indicators.
```

Code after:
		%[4]s

		Interactive text user interfaces manipulate the terminal cursor to redraw parts
		of the screen, which can be difficult for speech synthesizers or braille displays
		to accurately detect and read.

		For a more accessible experience, the GitHub CLI can provide a similar experience using
		non-interactive prompts for user input.

		To enable this experience, use one of the following methods:
		- Run %[1]sgh config set accessible_prompter enabled%[1]s
		- Set %[1]sGH_ACCESSIBLE_PROMPTER=enabled%[1]s environment variable

		%[5]s

		Motion-based spinners communicate in-progress activity by manipulating the
		terminal cursor to create a spinning effect, which may cause discomfort to users
		with motion sensitivity or miscommunicate information to speech synthesizers.

		For a more accessible experience, this interactivity can be disabled in favor
		of text-based progress indicators.

		To enable this experience, use one of the following methods:
		- Run %[1]sgh config set spinner disabled%[1]s
		- Set %[1]sGH_SPINNER_DISABLED=yes%[1]s environment variable

		%[6]s

		We invite you to join us in improving GitHub CLI accessibility by sharing your
		feedback and ideas through GitHub Accessibility feedback channels:

		%[7]s
	`, "`", title, color, prompter, spinner, feedback, webURL)
}
Repository: cli/cli
File: internal/prompter/prompter.go (Go)

Code before:
	var result int
	formOptions := []huh.Option[int]{}
	for i, o := range options {
		formOptions = append(formOptions, huh.NewOption(o, i))
	}

	form := p.newForm(
		huh.NewGroup(
			huh.NewSelect[int]().
				Title(prompt).
				Value(&result).
				Options(formOptions...),
		),
	)

	err := form.Run()
	return result, err
}

func (p *accessiblePrompter) MultiSelect(prompt string, defaults []string, options []string) ([]int, error) {
	var result []int
	formOptions := make([]huh.Option[int], len(options))
	for i, o := range options {
		formOptions[i] = huh.NewOption(o, i)
	}

	form := p.newForm(
		huh.NewGroup(
			huh.NewMultiSelect[int]().
				Title(prompt).
				Value(&result).
				Limit(len(options)).
				Options(formOptions...),
		),
	)

	if err := form.Run(); err != nil {
		return nil, err
	}

	return result, nil
}

func (p *accessiblePrompter) Input(prompt, defaultValue string) (string, error) {
	result := defaultValue
	prompt = fmt.Sprintf("%s (%s)", prompt, defaultValue)
	form := p.newForm(
		huh.NewGroup(
			huh.NewInput().
				Title(prompt).
				Value(&result),

Review comment (suggestion):
```suggestion
		// let's add its index to the result slice and huh
```


Code after:
func (p *accessiblePrompter) Select(prompt, _ string, options []string) (int, error) {
	var result int
	formOptions := []huh.Option[int]{}
	for i, o := range options {
		formOptions = append(formOptions, huh.NewOption(o, i))
	}

	form := p.newForm(
		huh.NewGroup(
			huh.NewSelect[int]().
				Title(prompt).
				Value(&result).
				Options(formOptions...),
		),
	)

	err := form.Run()
	return result, err
}

func (p *accessiblePrompter) MultiSelect(prompt string, defaults []string, options []string) ([]int, error) {
	var result []int
	formOptions := make([]huh.Option[int], len(options))
	for i, o := range options {
		// If this option is in the defaults slice,
		// let's add it's index to the result slice and huh
		// will treat it as a default selection.
		// TODO: does an invalid default value constitute a panic?
		if slices.Contains(defaults, o) {
			result = append(result, i)
		}

		formOptions[i] = huh.NewOption(o, i)
	}

	form := p.newForm(
		huh.NewGroup(
			huh.NewMultiSelect[int]().
				Title(prompt).
				Value(&result).
				Limit(len(options)).
				Options(formOptions...),
		),
	)

	if err := form.Run(); err != nil {
		return nil, err
	}

	return result, nil
}
Repository: cli/cli
File: internal/prompter/test.go (Go)

Code before:
	if len(errs) > 0 {
		m.t.Helper()
		m.t.Errorf("%d unmatched calls to %s", len(errs), strings.Join(errs, ","))
	}
}

func AssertOptions(t *testing.T, expected, actual []string) {
	assert.Equal(t, expected, actual)
}

func IndexFor(options []string, answer string) (int, error) {
	for ix, a := range options {
		if a == answer {
			return ix, nil
		}
	}
	return -1, NoSuchAnswerErr(answer, options)
}

func NoSuchPromptErr(prompt string) error {
	return fmt.Errorf("no such prompt '%s'", prompt)
}

func NoSuchAnswerErr(answer string, options []string) error {
	return fmt.Errorf("no such answer '%s' in [%s]", answer, strings.Join(options, ", "))
}


Review comment (suggestion):
**thought:** this will return an error if any answer is not an available option.  `IndexFor` returns `-1` for the `int` index of the answer, which is necessary being a literal with anything `0` or more being a valid index while this has `[]int`.

makes sense

Code after:
	if len(errs) > 0 {
		m.t.Helper()
		m.t.Errorf("%d unmatched calls to %s", len(errs), strings.Join(errs, ","))
	}
}

func AssertOptions(t *testing.T, expected, actual []string) {
	assert.Equal(t, expected, actual)
}

func IndexFor(options []string, answer string) (int, error) {
	for ix, a := range options {
		if a == answer {
			return ix, nil
		}
	}
	return -1, NoSuchAnswerErr(answer, options)
}

func IndexesFor(options []string, answers ...string) ([]int, error) {
	indexes := make([]int, len(answers))
	for i, answer := range answers {
		index, err := IndexFor(options, answer)
		if err != nil {
			return nil, err
		}
		indexes[i] = index
	}
	return indexes, nil
}

func NoSuchPromptErr(prompt string) error {
	return fmt.Errorf("no such prompt '%s'", prompt)
}

func NoSuchAnswerErr(answer string, options []string) error {
	return fmt.Errorf("no such answer '%s' in [%s]", answer, strings.Join(options, ", "))
}
Repository: cli/cli
File: pkg/cmd/issue/view/view.go (Go)

Code before:
	}

	cmd.Flags().BoolVarP(&opts.WebMode, "web", "w", false, "Open an issue in the browser")
	cmd.Flags().BoolVarP(&opts.Comments, "comments", "c", false, "View issue comments")
	cmdutil.AddJSONFlags(cmd, &opts.Exporter, api.IssueFields)

	return cmd
}

var defaultFields = []string{
	"number", "url", "state", "createdAt", "title", "body", "author", "milestone",
	"assignees", "labels", "reactionGroups", "lastComment", "stateReason",
}

func viewRun(opts *ViewOptions) error {
	httpClient, err := opts.HttpClient()
	if err != nil {
		return err
	}

	baseRepo, err := opts.BaseRepo()
	if err != nil {
		return err
	}

	fields := set.NewStringSet()
	if opts.Exporter != nil {
		fields.AddValues(opts.Exporter.Fields())
	} else if opts.WebMode {
		fields.Add("url")
	} else {
		fields.AddValues(defaultFields)
		if opts.Comments {
			fields.Add("comments")
			fields.Remove("lastComment")
		}

		// TODO projectsV1Deprecation
		// Remove this section as we should no longer add projectCards
		if opts.Detector == nil {
			cachedClient := api.NewCachedHTTPClient(httpClient, time.Hour*24)
			opts.Detector = fd.NewDetector(cachedClient, baseRepo.RepoHost())
		}

		projectsV1Support := opts.Detector.ProjectsV1()
		if projectsV1Support == gh.ProjectsV1Supported {
			fields.Add("projectCards")
		}
	}

	opts.IO.DetectTerminalTheme()

Review comment (suggestion):
I get the reason for this rename, but this is currently consistent with `issue edit`. So, let's please revert it to `lookupFields`.

Code after:

	cmd.Flags().BoolVarP(&opts.WebMode, "web", "w", false, "Open an issue in the browser")
	cmd.Flags().BoolVarP(&opts.Comments, "comments", "c", false, "View issue comments")
	cmdutil.AddJSONFlags(cmd, &opts.Exporter, api.IssueFields)

	return cmd
}

var defaultFields = []string{
	"number", "url", "state", "createdAt", "title", "body", "author", "milestone",
	"assignees", "labels", "reactionGroups", "lastComment", "stateReason",
}

func viewRun(opts *ViewOptions) error {
	httpClient, err := opts.HttpClient()
	if err != nil {
		return err
	}

	baseRepo, err := opts.BaseRepo()
	if err != nil {
		return err
	}

	lookupFields := set.NewStringSet()
	if opts.Exporter != nil {
		lookupFields.AddValues(opts.Exporter.Fields())
	} else if opts.WebMode {
		lookupFields.Add("url")
	} else {
		lookupFields.AddValues(defaultFields)
		if opts.Comments {
			lookupFields.Add("comments")
			lookupFields.Remove("lastComment")
		}

		// TODO projectsV1Deprecation
		// Remove this section as we should no longer add projectCards
		if opts.Detector == nil {
			cachedClient := api.NewCachedHTTPClient(httpClient, time.Hour*24)
			opts.Detector = fd.NewDetector(cachedClient, baseRepo.RepoHost())
		}

		projectsV1Support := opts.Detector.ProjectsV1()
		if projectsV1Support == gh.ProjectsV1Supported {
			lookupFields.Add("projectCards")
		}
	}

	opts.IO.DetectTerminalTheme()
Repository: cli/cli
File: pkg/cmd/issue/view/view.go (Go)

Code before:
			cachedClient := api.NewCachedHTTPClient(httpClient, time.Hour*24)
			opts.Detector = fd.NewDetector(cachedClient, baseRepo.RepoHost())
		}

		projectsV1Support := opts.Detector.ProjectsV1()
		if projectsV1Support == gh.ProjectsV1Supported {
			fields.Add("projectCards")
		}
	}

	opts.IO.DetectTerminalTheme()

	opts.IO.StartProgressIndicator()
	defer opts.IO.StopProgressIndicator()

	fields.Add("id")

	issue, err := issueShared.FindIssueOrPR(httpClient, baseRepo, opts.IssueNumber, fields.ToSlice())
	if err != nil {
		var partialLoadErr *issueShared.PartialLoadError
		if opts.Exporter == nil && errors.As(err, &partialLoadErr) {
			fmt.Fprintf(opts.IO.ErrOut, "warning: %s\n", partialLoadErr.Error())
		} else {
			return err
		}
	}

	if fields.Contains("comments") {
		// FIXME: this re-fetches the comments connection even though the initial set of 100 were
		// fetched in the previous request.
		err := preloadIssueComments(httpClient, baseRepo, issue)
		if err != nil {
			return err
		}
	}

	if fields.Contains("closedByPullRequestsReferences") {
		err := preloadClosedByPullRequestsReferences(httpClient, baseRepo, issue)
		if err != nil {
			return err
		}
	}

	opts.IO.StopProgressIndicator()

	if opts.WebMode {
		openURL := issue.URL
		if opts.IO.IsStdoutTTY() {
			fmt.Fprintf(opts.IO.ErrOut, "Opening %s in your browser.\n", text.DisplayURL(openURL))
		}
		return opts.Browser.Browse(openURL)

Review comment (suggestion):
Currently, as of recent changes, now we do not treat partial errors differently, and just bail out if error is not `nil`. I don't think bringing back the old behaviour is the right decision here. So, let's just return the `err` and avoid dealing with partial errors. Also, it's not a well-tested path and it's better to leave it as it was.

Code after:
			opts.Detector = fd.NewDetector(cachedClient, baseRepo.RepoHost())
		}

		projectsV1Support := opts.Detector.ProjectsV1()
		if projectsV1Support == gh.ProjectsV1Supported {
			lookupFields.Add("projectCards")
		}
	}

	opts.IO.DetectTerminalTheme()

	opts.IO.StartProgressIndicator()
	defer opts.IO.StopProgressIndicator()

	lookupFields.Add("id")

	issue, err := issueShared.FindIssueOrPR(httpClient, baseRepo, opts.IssueNumber, lookupFields.ToSlice())
	if err != nil {
		return err
	}

	if lookupFields.Contains("comments") {
		// FIXME: this re-fetches the comments connection even though the initial set of 100 were
		// fetched in the previous request.
		err := preloadIssueComments(httpClient, baseRepo, issue)
		if err != nil {
			return err
		}
	}

	if lookupFields.Contains("closedByPullRequestsReferences") {
		err := preloadClosedByPullRequestsReferences(httpClient, baseRepo, issue)
		if err != nil {
			return err
		}
	}

	opts.IO.StopProgressIndicator()

	if opts.WebMode {
		openURL := issue.URL
		if opts.IO.IsStdoutTTY() {
			fmt.Fprintf(opts.IO.ErrOut, "Opening %s in your browser.\n", text.DisplayURL(openURL))
		}
		return opts.Browser.Browse(openURL)
	}

	if err := opts.IO.StartPager(); err != nil {
		fmt.Fprintf(opts.IO.ErrOut, "error starting pager: %v\n", err)
	}
	defer opts.IO.StopPager()