Skip to content

Conversation

MoonSupport
Copy link
Contributor

@MoonSupport MoonSupport commented Jul 30, 2020

Description of the Change

I used the log-symbols instead of the code for win32.

Alternate Designs

No alternate designs

Benefits

Mocha already uses log-symbols, so we can remove duplicated code and maintain consistency.

Possible Drawbacks

I also deleted the dot case, But I don't know if the size difference between dot for win32(.) and dot for the others() is a problem.

So I think this might be a problem.

Applicable issues

The above issue is not directly related to the PR.

But I think you may want to use alphabets rather than log-symbols to resolve the issue.

like this

success : v or o
fail : x

Please leave a good comment or question at any time :)

  • Is this a breaking change (major release)? no
  • Is it an enhancement (minor release)? maybe, yes
  • Is it a bug fix, or does it not impact production code (patch release)? no

@jsf-clabot
Copy link

jsf-clabot commented Jul 30, 2020

CLA assistant check
All committers have signed the CLA.

@boneskull boneskull added type: bug a defect, confirmed by a maintainer semver-major implementation requires increase of "major" version number; "breaking changes" labels Jul 30, 2020
Copy link
Member

@boneskull boneskull left a comment

Choose a reason for hiding this comment

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

Thanks. this looks good to me, but I think we need to consider it a breaking change, so it should wait for v9.0.0.

Can you please sign the CLA?

@MoonSupport
Copy link
Contributor Author

MoonSupport commented Jul 30, 2020

Ok, I did it :)

@outsideris
Copy link
Contributor

I've re-run AppVeyor and it passed.

@juergba juergba added this to the v9.0.0 milestone May 7, 2021
@juergba
Copy link
Contributor

juergba commented May 21, 2021

@MoonSupport could you rebase to master, please? Thanks.
If all tests have passed, I will merge this PR.

@MoonSupport
Copy link
Contributor Author

@juergba Done :)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 94.287% when pulling f8dd915 on MoonSupport:master into 641970d on mochajs:master.

@juergba juergba added the run-browser-test run browser tests on forked PR if code is safe label May 21, 2021
@github-actions github-actions bot removed the run-browser-test run browser tests on forked PR if code is safe label May 21, 2021
Copy link
Contributor

@juergba juergba left a comment

Choose a reason for hiding this comment

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

@MoonSupport thank you for this PR.

@juergba juergba merged commit 7c3daea into mochajs:master May 21, 2021
@juergba juergba added type: cleanup a refactor and removed type: bug a defect, confirmed by a maintainer labels May 21, 2021
@jimmywarting
Copy link

Currently looking into all the dependencies... and one of your dependencies are log-symbol

log-symbols seems like a perfect package if it had just only precomputed the values instead of using chalk as an dependency
the size could have been a lot smaller if it was just written with:

import isUnicodeSupported from 'is-unicode-supported';

const main = {
  info: '\x1B[34mℹ\x1B[39m',
  success: '\x1B[32m✔\x1B[39m',
  warning: '\x1B[33m⚠\x1B[39m',
  error: '\x1B[31m✖\x1B[39m'
};

const fallback = {
  info: '\x1B[34mi\x1B[39m',
  success: '\x1B[32m√\x1B[39m',
  warning: '\x1B[33m‼\x1B[39m',
  error: '\x1B[31m×\x1B[39m'
};

const logSymbols = isUnicodeSupported() ? main : fallback;

export default logSymbols;

Here is one tough: how about inlining this symbols into mocha itself?

@juergba
Copy link
Contributor

juergba commented Sep 4, 2021

@jimmywarting thank you. The size of the code is not the only criteria.

All those codes/numbers (unicode?) isn't the detail level I want to deal with. We need a package which works for different OS/browsers and is well maintained by someone else, not by Mocha.
Time is the main criteria when maintaining a package like Mocha. So inlining those symbols isn't a good idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major implementation requires increase of "major" version number; "breaking changes" type: cleanup a refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants