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

Refactoring: Consuming log-symbols alternate to code for win32 in reporters/base. #4389

Merged
merged 1 commit into from
May 21, 2021
Merged

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
Contributor

@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