Skip to content
This repository was archived by the owner on Apr 19, 2024. It is now read-only.

Added WithPageSize AskOpt #214

Merged
merged 5 commits into from
Jun 3, 2019
Merged

Added WithPageSize AskOpt #214

merged 5 commits into from
Jun 3, 2019

Conversation

AlecAivazis
Copy link
Owner

@AlecAivazis AlecAivazis commented May 31, 2019

This PR is part of the work summarized in #201. It adds the survey.WithPageSize AskOpt that replaces the modification of survey.PageSize as the way to modify the default page size for prompts.

Since we now have to have a place for "global" configuration of prompts, I had to change the Prompt interface to include a reference to the config.

@AlecAivazis AlecAivazis requested a review from coryb May 31, 2019 23:26
@AlecAivazis AlecAivazis mentioned this pull request Jun 1, 2019
Copy link
Collaborator

@coryb coryb left a comment

Choose a reason for hiding this comment

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

I see the if pageSize == 0 logic repeated a few times for pagable prompts. I wonder if it would make sense to factor some of this out into a Pagable struct with composition, something like:

type Select struct {
  Pagable
  ...
}
type Pagable struct {
  PageSize int
}
func (p *Pagable) pageSize(config *PromptConfig) int {
  if p.PageSize == 0 {
    return config.PageSize
  }
  return p.PageSize
}

Then in your code:

func (s *Select) OnChange(key run, config *PromptConfig) bool {
  ...
  pageSize := s.pageSize(config)
  ...
}

Going to approve this PR since I dont think the above changes are necessary, just something to think about.

@AlecAivazis
Copy link
Owner Author

Yea I thought about that while I was putting this together but it felt like a lot of ceremony for an if statement, especially considering I'd still have to pass the config in. There could be an argument for encapsulating the paginate function into a method. I'll take a look and see if its worth it before merging - thanks for the review!

@AlecAivazis
Copy link
Owner Author

So I tried to consolidate the paginate logic into a method on a Pager interface like you suggested. Unfortunately, since you can't set promoted fields, this would force the API to be:

Select{
    Pager{ PageSize: 7 }
}

which is just a little too verbose for me

@AlecAivazis AlecAivazis merged commit d5ae5ee into master Jun 3, 2019
@AlecAivazis AlecAivazis deleted the page-size-ask-opt branch June 3, 2019 06:02
@coryb
Copy link
Collaborator

coryb commented Jun 3, 2019

Okay, yeah, I agree, forgot about that aspect. We could minimize this issue by having a NewSelect etc, with variadic args, ie NewSelect(opts ...func(*Select)) and WithPageSize(s *Select). Not sure if there are other issues that would be resolved or made worse by this direction though.

@AlecAivazis
Copy link
Owner Author

AlecAivazis commented Jun 3, 2019

Yea that's something to keep in mind. It starts to become very verbose and I think so long as we have sane interpretations of a zero value, we should keep the struct-based API as I think its much simpler for people to grok quickly

@AlecAivazis
Copy link
Owner Author

It just occurred to me that it's also very possible that sort of API is what a solution to #105 would look like.

siredmar pushed a commit to siredmar/survey that referenced this pull request Feb 20, 2020
* added PromptConfig to Prompt interface

* documented WithPageSize

* selects will pull page size from global config

* removed unused variable

* fixed display problem on key press
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants