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

fix: Fix unicode symbols compatibility on Windows #248

Merged
merged 2 commits into from
Sep 7, 2017
Merged

Conversation

luftywiranda13
Copy link
Collaborator

@luftywiranda13 luftywiranda13 commented Sep 4, 2017

Replace emojis with symbols from log-symbols

Closes: #247 #248

Replace emojis with symbols from `log-symbols`
@luftywiranda13
Copy link
Collaborator Author

I think it's better to wait for #242 to be merged first

@sudo-suhas
Copy link
Collaborator

Don't worry about it. You can go ahead with this. I'll rebase and do the required changes.

sudo-suhas
sudo-suhas previously approved these changes Sep 4, 2017
Copy link
Collaborator

@sudo-suhas sudo-suhas left a comment

Choose a reason for hiding this comment

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

LGTM

@luftywiranda13
Copy link
Collaborator Author

cool! But only @okonet who can force merge this

@@ -32,7 +33,7 @@ describe('runScript', () => {
try {
await res[0].task()
} catch (err) {
expect(err.message).toMatch(`🚫 test got an unexpected error.
expect(err.message).toMatch(`${logSymbols.error} test got an unexpected error.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

agree! do it in this PR or on separate PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

@luftywiranda13 doesn't matter to me TBH.

@sudo-suhas could you help with those examples and create a PR that would "fix" them before this one is merged?

Copy link
Collaborator

@sudo-suhas sudo-suhas Sep 4, 2017

Choose a reason for hiding this comment

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

I am a bit swamped at the moment. Trying to finish something at work. I can do it tomorrow. I think I'd prefer optimising jest related things in a separate PR after all the changes I have in the pipeline:

  • eslint-plugin-lodash
  • fix failing tests on windows
  • rebase dedent PR
  • getConfig - _normalized property
  • try pify

edit: 'fix failing tests on windows' takes highest priority. I actually can't run tests for my changes otherwise. And gotta make it all green 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

+1 on pify 👍

@@ -146,7 +147,7 @@ describe('runScript', () => {
await taskPromise
} catch (err) {
expect(err.message)
.toMatch(`🚫 mock-fail-linter found some errors. Please fix them and try committing again.
.toMatch(`${logSymbols.error} mock-fail-linter found some errors. Please fix them and try committing again.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@okonet
Copy link
Collaborator

okonet commented Sep 4, 2017

Let's wait with merging this one until appveyor integration is green.

@okonet okonet changed the title refactor: improve unicode symbols compatibility on Windows fix: Fix unicode symbols compatibility on Windows Sep 5, 2017
@codecov
Copy link

codecov bot commented Sep 6, 2017

Codecov Report

Merging #248 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #248      +/-   ##
==========================================
+ Coverage   90.12%   90.18%   +0.06%     
==========================================
  Files          10       10              
  Lines         162      163       +1     
  Branches       23       23              
==========================================
+ Hits          146      147       +1     
  Misses         15       15              
  Partials        1        1
Impacted Files Coverage Δ
src/runScript.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8a585a...53ada13. Read the comment docs.

@sudo-suhas
Copy link
Collaborator

I tested this out using yarn link and things work fine. As it happens, I am on windows 10 and it has better support for unicode so could not notice any change with and without this change. Will try it out on windows 8 machine if possible.

@okonet
Copy link
Collaborator

okonet commented Sep 7, 2017

I'm gonna merge this one. If you find issues, please report them separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants