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

Add auto colors functionality #296

Merged
merged 12 commits into from Oct 10, 2022

Conversation

eliasm307
Copy link
Contributor

@eliasm307 eliasm307 commented Nov 19, 2021

Resolves #279

@eliasm307 eliasm307 force-pushed the feat/random-colors branch 2 times, most recently from c9739e3 to 2118d3f Compare November 19, 2021 01:46
@coveralls
Copy link

coveralls commented Nov 19, 2021

Coverage Status

Coverage remained the same at 100.0% when pulling 7929986 on eliasm307:feat/random-colors into f6a588e on open-cli-tools:main.

@clement-at-beekast

This comment was marked as outdated.

@gustavohenke gustavohenke self-requested a review February 3, 2022 01:21
@paescuj paescuj changed the title Add auto colors functionality to resolve #279 Add auto colors functionality May 20, 2022
@paescuj
Copy link
Collaborator

paescuj commented Jun 21, 2022

@eliasm307 Thanks for this pull request! I've now rebased it on the latest state of the main branch. I've only made the necessary changes required for TypeScript & ESM and otherwise kept the original logic the same. Let's see if @gustavohenke and / or me can find the time to review this pull request soon.

@paescuj paescuj self-requested a review June 21, 2022 10:08
@paescuj paescuj force-pushed the feat/random-colors branch 2 times, most recently from c259335 to f57afd7 Compare June 21, 2022 10:53
Copy link
Member

@gustavohenke gustavohenke left a comment

Choose a reason for hiding this comment

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

Overall thought that doesn't belong to one specific line:
What do you @eliasm307 and @paescuj think of --prefix-colors taking a special keyword auto that triggers this behaviour? So that this doesn't require implementing/specifying --color.

E.g. concurrently -c auto,red,auto npm:lint npm:test npm:build generates an auto colour for npm:lint, red for npm:test, and another auto colour for npm:build (but excluding red)

README.md Outdated Show resolved Hide resolved
src/prefix-color-selector.ts Outdated Show resolved Hide resolved
src/prefix-color-selector.ts Outdated Show resolved Hide resolved
src/prefix-color-selector.ts Outdated Show resolved Hide resolved
src/prefix-color-selector.ts Outdated Show resolved Hide resolved
@eliasm307
Copy link
Contributor Author

@gustavohenke thats an interesting suggestion, I think it could fit some use cases where a user wants partial control of the process colours.

However, for me the motivation for this was that I didn't really mind about the colours, as long as they are different. If other users also don't mind and they are trying to use this to spawn several processes then the command ends up with noise like "auto,auto,auto..." which isn't a good experience imo.

How would you feel about supporting both? So one option of all auto colours or being able to specify colours per process with an option to delegate?

@gustavohenke
Copy link
Member

command ends up with noise like "auto,auto,auto..." which isn't a good experience imo.

We could rely on the current behaviour of the last input colour getting repeated when there are less colours than commands. So a single auto would do it if you had 20 commands.

Also sorry for reviewing this harshly, I didn't notice it was from a bunch of months back! 😱

@paescuj
Copy link
Collaborator

paescuj commented Jun 30, 2022

We could rely on the current behaviour of the last input colour getting repeated when there are less colours than commands. So a single auto would do it if you had 20 commands.

I've already considered this too.
The advantage to this would be that we don't need to introduce another option.
The disadvantage in my opinion is that it might not be very intuitive at first glance. For example if I want to have auto colors for all commands except the second one, then I had to use auto,red,auto. I think this is okay, but we would need to document it clearly and provide a few examples.

@eliasm307
Copy link
Contributor Author

eliasm307 commented Jul 22, 2022

command ends up with noise like "auto,auto,auto..." which isn't a good experience imo.

We could rely on the current behaviour of the last input colour getting repeated when there are less colours than commands. So a single auto would do it if you had 20 commands.

Also sorry for reviewing this harshly, I didn't notice it was from a bunch of months back! 😱

@gustavohenke Hey, I didnt think it was harsh, it was good feedback, thanks for taking the time to review it 🙂

@eliasm307
Copy link
Contributor Author

Example output:
image

@eliasm307 eliasm307 force-pushed the feat/random-colors branch 6 times, most recently from 5bdf259 to 8200972 Compare July 23, 2022 10:10
Copy link
Collaborator

@paescuj paescuj left a comment

Choose a reason for hiding this comment

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

Thanks @eliasm307 for your great work here!
It looks good to me 👍
I just did some small enhancements concerning docs & comments and a little cleanup.

Co-authored-by: Pascal Jufer <pascal-jufer@bluewin.ch>
…Colors'

I'm neither happy with extending expect nor with having expect in a
external function, so I'm using 'it.each' to solve this
@paescuj
Copy link
Collaborator

paescuj commented Oct 2, 2022

What do you think @gustavohenke?

Copy link
Member

@gustavohenke gustavohenke left a comment

Choose a reason for hiding this comment

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

🤦 I had reviewed just hadn't submitted

src/prefix-color-selector.spec.ts Outdated Show resolved Hide resolved
@paescuj paescuj merged commit b56fbdd into open-cli-tools:main Oct 10, 2022
@gustavohenke
Copy link
Member

Published in v7.5.0!

renovate bot added a commit to fwouts/previewjs that referenced this pull request Oct 22, 2022
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [concurrently](https://togithub.com/open-cli-tools/concurrently) |
[`^7.4.0` ->
`^7.5.0`](https://renovatebot.com/diffs/npm/concurrently/7.4.0/7.5.0) |
[![age](https://badges.renovateapi.com/packages/npm/concurrently/7.5.0/age-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://badges.renovateapi.com/packages/npm/concurrently/7.5.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://badges.renovateapi.com/packages/npm/concurrently/7.5.0/compatibility-slim/7.4.0)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://badges.renovateapi.com/packages/npm/concurrently/7.5.0/confidence-slim/7.4.0)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>open-cli-tools/concurrently</summary>

###
[`v7.5.0`](https://togithub.com/open-cli-tools/concurrently/releases/tag/v7.5.0)

[Compare
Source](https://togithub.com/open-cli-tools/concurrently/compare/v7.4.0...v7.5.0)

#### What's Changed

- Add auto colors functionality by
[@&#8203;eliasm307](https://togithub.com/eliasm307),
[@&#8203;paescuj](https://togithub.com/paescuj),
[@&#8203;gustavohenke](https://togithub.com/gustavohenke) in
[open-cli-tools/concurrently#296
- Fix `onFinish` signature when using exactOptionalPropertyTypes by
[@&#8203;Baune8D](https://togithub.com/Baune8D) in
[open-cli-tools/concurrently#372

**Full Changelog**:
open-cli-tools/concurrently@v7.4.0...v7.5.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click
this checkbox.

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/fwouts/previewjs).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4yNDEuNyIsInVwZGF0ZWRJblZlciI6IjMyLjI0MS43In0=-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
renovate bot added a commit to defenseunicorns/zarf that referenced this pull request Oct 22, 2022
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [concurrently](https://togithub.com/open-cli-tools/concurrently) |
[`7.4.0` ->
`7.5.0`](https://renovatebot.com/diffs/npm/concurrently/7.4.0/7.5.0) |
[![age](https://badges.renovateapi.com/packages/npm/concurrently/7.5.0/age-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://badges.renovateapi.com/packages/npm/concurrently/7.5.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://badges.renovateapi.com/packages/npm/concurrently/7.5.0/compatibility-slim/7.4.0)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://badges.renovateapi.com/packages/npm/concurrently/7.5.0/confidence-slim/7.4.0)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>open-cli-tools/concurrently</summary>

###
[`v7.5.0`](https://togithub.com/open-cli-tools/concurrently/releases/tag/v7.5.0)

[Compare
Source](https://togithub.com/open-cli-tools/concurrently/compare/v7.4.0...v7.5.0)

#### What's Changed

- Add auto colors functionality by
[@&#8203;eliasm307](https://togithub.com/eliasm307),
[@&#8203;paescuj](https://togithub.com/paescuj),
[@&#8203;gustavohenke](https://togithub.com/gustavohenke) in
[open-cli-tools/concurrently#296
- Fix `onFinish` signature when using exactOptionalPropertyTypes by
[@&#8203;Baune8D](https://togithub.com/Baune8D) in
[open-cli-tools/concurrently#372

**Full Changelog**:
open-cli-tools/concurrently@v7.4.0...v7.5.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click
this checkbox.

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/defenseunicorns/zarf).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4yNDEuNyIsInVwZGF0ZWRJblZlciI6IjMyLjI0MS43In0=-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Noxsios pushed a commit to defenseunicorns/zarf that referenced this pull request Mar 8, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [concurrently](https://togithub.com/open-cli-tools/concurrently) |
[`7.4.0` ->
`7.5.0`](https://renovatebot.com/diffs/npm/concurrently/7.4.0/7.5.0) |
[![age](https://badges.renovateapi.com/packages/npm/concurrently/7.5.0/age-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://badges.renovateapi.com/packages/npm/concurrently/7.5.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://badges.renovateapi.com/packages/npm/concurrently/7.5.0/compatibility-slim/7.4.0)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://badges.renovateapi.com/packages/npm/concurrently/7.5.0/confidence-slim/7.4.0)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>open-cli-tools/concurrently</summary>

###
[`v7.5.0`](https://togithub.com/open-cli-tools/concurrently/releases/tag/v7.5.0)

[Compare
Source](https://togithub.com/open-cli-tools/concurrently/compare/v7.4.0...v7.5.0)

#### What's Changed

- Add auto colors functionality by
[@&#8203;eliasm307](https://togithub.com/eliasm307),
[@&#8203;paescuj](https://togithub.com/paescuj),
[@&#8203;gustavohenke](https://togithub.com/gustavohenke) in
[open-cli-tools/concurrently#296
- Fix `onFinish` signature when using exactOptionalPropertyTypes by
[@&#8203;Baune8D](https://togithub.com/Baune8D) in
[open-cli-tools/concurrently#372

**Full Changelog**:
open-cli-tools/concurrently@v7.4.0...v7.5.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click
this checkbox.

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/defenseunicorns/zarf).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4yNDEuNyIsInVwZGF0ZWRJblZlciI6IjMyLjI0MS43In0=-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
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.

FR: Random Colors
5 participants