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

Refactor formatter/ANSIColor #1589

Merged
merged 22 commits into from
Dec 1, 2021
Merged

Conversation

aurelien-reeves
Copy link
Contributor

@aurelien-reeves aurelien-reeves commented Nov 25, 2021

Description

Fixes #1583

Type of change

  • Refactoring (improvements to code design or tooling that don't change behaviour)

Checklist:

Your PR is ready for review once the following checklist is
complete. You can also add some checks if you want to.

  • Tests have been added for any changes to behaviour of the code
  • New and existing tests are passing locally and on CI
  • bundle exec rubocop reports no offenses
  • RDoc comments have been updated
  • CHANGELOG.md has been updated

@aurelien-reeves aurelien-reeves marked this pull request as draft November 25, 2021 10:43
The gem is not a dependency of cucumber, but its installation was
automatically detected by code.

This gem is not very popular, and unmaintained, and is used only to make
sure 256 colors are available on the user's terminal.

When the gem is not installed, it fallback to defining the 'grey' method
anyway.

Removing that gem result in declaring the 'grey' method as part of
'Cucumber::Term::ANSIColor': it did not make sense anymore to keep it
away from that module which already defined all other term colors.
Putting a call to a regexp before a call to 'Regexp.last_match' may
improve the readibility of the code as it follows a more natural
reading direction
@aurelien-reeves
Copy link
Contributor Author

aurelien-reeves commented Nov 29, 2021

Do we still need the cukes method in lib/cucumber/formatter/ansicolor.rb? (Refs.

)

It looks like there were part of old warning mesages which are not displayed anymore: 05252af

And they are unused at the moment

@aurelien-reeves aurelien-reeves marked this pull request as ready for review November 30, 2021 14:21
@aurelien-reeves
Copy link
Contributor Author

Yes :D ❤️

Is this an answer to the question "do we remove those 'cukes' methods"?

@luke-hill
Copy link
Contributor

Sorry I meant this is the better way to do it (Define method vs eval).

@aurelien-reeves
Copy link
Contributor Author

I think we can let those cukes method for now

@aurelien-reeves
Copy link
Contributor Author

I know you already approved it
I pushed some documentation updates in the meantime. I just want to be sure it is right

Copy link
Contributor

@luke-hill luke-hill left a comment

Choose a reason for hiding this comment

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

Quite a bit of this looks new and I didn't quite understand. So I've only reviewed part of it.

lib/cucumber/formatter/ansicolor.rb Show resolved Hide resolved
lib/cucumber/formatter/ansicolor.rb Outdated Show resolved Hide resolved
lib/cucumber/formatter/ansicolor.rb Show resolved Hide resolved
lib/cucumber/term/ansicolor.rb Outdated Show resolved Hide resolved
spec/cucumber/formatter/ansicolor_spec.rb Outdated Show resolved Hide resolved
@aurelien-reeves aurelien-reeves force-pushed the refactor-formatter-ansicolor branch from 694fab7 to 86ddc10 Compare December 1, 2021 14:44
@aurelien-reeves
Copy link
Contributor Author

Quite a bit of this looks new and I didn't quite understand. So I've only reviewed part of it.

It is supposed to be easier to understand than it was before actually 😓

lib/cucumber/formatter/ansicolor.rb Outdated Show resolved Hide resolved
lib/cucumber/formatter/ansicolor.rb Outdated Show resolved Hide resolved
aurelien-reeves and others added 2 commits December 1, 2021 16:21
Co-authored-by: Matt Wynne <matt@cucumber.io>
Co-authored-by: Matt Wynne <matt@cucumber.io>
@aurelien-reeves aurelien-reeves merged commit 0ed3faf into main Dec 1, 2021
@luke-hill luke-hill deleted the refactor-formatter-ansicolor branch December 2, 2021 08:57
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.

Refactor formatter/ansicolor.rb
3 participants