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

Move Spec context state into Spec::CLI #14259

Merged

Conversation

HertzDevil
Copy link
Contributor

Follow-up to #14170. This includes the global variables inside Spec::RootContext.

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

thought: I'm wondering if the class name CLI is actually a good fit, particularly with these changes. It seems to be more than a command line interface. Maybe a more generic term like Run might be better?

question: It seems the refactoring from print_results to prints_summary? is a prerequisite but not directly related to the context state refactor. Could we separate this?

@HertzDevil
Copy link
Contributor Author

CLI is at the moment still :nodoc:, we could change it any time we want.

prints_summary? reflects the fact that formatters don't depend on a context. Any other refactoring would introduce an equally "unrelated" change.

@straight-shoota
Copy link
Member

Not sure I understand what you mean. Wouldn't it be possible to apply the refactor from print_results to prints_summary? without the other changes? If it is, we should do it first, then move the other stuff around.

@straight-shoota straight-shoota added this to the 1.13.0 milestone Mar 27, 2024
@straight-shoota straight-shoota merged commit a318485 into crystal-lang:master Apr 13, 2024
58 checks passed
@HertzDevil HertzDevil deleted the refactor/spec-root-context branch April 15, 2024 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants