Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

this enables the collector to access the prompt object #195

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

doriantaylor
Copy link

@doriantaylor doriantaylor commented Nov 17, 2023

Describe the change

Adds a read-only accessor to the @prompt member of TTY::Prompt::AnswerCollector

Why are we doing this?

To prevent ugly compromises in the code when we want to access the prompt without modifying the state of the answer collector.

Benefits

See above.

Drawbacks

It's literally one line of code that says attr_reader :prompt.

Requirements

  • Tests written & passing locally?
  • Code style checked?
  • Rebased with master branch?
  • Documentation updated?
  • Changelog updated?

It's literally one line of code.

@piotrmurach
Copy link
Owner

Hi Dorian,

Thanks for this PR.

To prevent ugly compromises in the code when we want to access the prompt without modifying the state of the answer collector.

Please provide an example when this is the case.

It's literally one line of code.

Let's take a step back for a moment. I maintain many Ruby projects and over the years established some helpful processes. The checklist is one of them. I have a responsibility to release quality software. This is a project that many people rely on and use. How would they find out about this addition, which is an extension of public API if no documentation is updated? More importantly, it would be good to have a test case that proves and cements the API so that nothing regresses in the future. So I'd encourage you to consider the checklist more carefully with an eye towards the longevity of the project and maintenance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants