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

[WIP] Colorize file generation #906

Closed
wants to merge 2 commits into from
Closed

[WIP] Colorize file generation #906

wants to merge 2 commits into from

Conversation

cllns
Copy link
Member

@cllns cllns commented Feb 26, 2018

Here's a work-in-progress for colorizing the CLI commands:

image

You can see what changes will be necessary to the CLI specs. It's a lot of manual work (or could be automated) but it'll cause a lot of diff noise, and make it much harder to edit the output of the specs in the future.

We (@jodosha and I) have talked about extracting the file generation commands into hanami/cli. If we did that, then we could test these exact CLI things, without repeating the shellcodes a bunch of times in this repo. We could even write a helper to strip the shellcodes, if we wanted.

Thoughts? @jodosha @hanami/core?

@@ -8,7 +8,7 @@ end

gem 'i18n'

gem 'hanami-utils', '~> 1.1', require: false, git: 'https://github.com/hanami/utils.git', branch: '1.1.x'
gem 'hanami-utils', '~> 1.1', require: false, git: 'https://github.com/hanami/utils.git', branch: 'coloured_output'
Copy link
Member Author

@cllns cllns Feb 26, 2018

Choose a reason for hiding this comment

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

Temporary, until this branch is merged in.

if OPERATION_COLORS.key?(operation)
_colorize_and_justify(operation)
else
operation.to_s.rjust(COLUMN_WIDTH)
Copy link
Member Author

Choose a reason for hiding this comment

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

We could just colorize other stuff to black, to get rid of this conditional.

Copy link
Member

Choose a reason for hiding this comment

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

@cllns This assumes a black terminal background.

@jodosha
Copy link
Member

jodosha commented Apr 4, 2018

@cllns This feature is just great to have, but it comes with the cost of changing all the specs under spec/integration/cli.

My suggestion is to:

  1. Close this PR
  2. Work on hanami-cli (after v0.2.0) to add the colorization feature over there. It has to be turned off by default
  3. Work on a new PR to add a private setting for Hanami (see Snippet 1)
  4. On the same PR generate new projects with colorization turned off. This avoid us to rewrite all the specs under spec/integration/cli and it keeps stack traces clean in case of CI failure.
  5. Still on the same PR, add colorization feature (from this PR), alongside with new integration tests with colorization turned on (only for these few specs).

Snippet 1

# config/environment.rb
Hanami.configure do
  # ...
  cli colorize: false
end

@cllns
Copy link
Member Author

cllns commented Apr 4, 2018

👍

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