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

Refactor console special characters #1438

Merged
merged 2 commits into from
Jan 9, 2017

Conversation

maciej-ka
Copy link
Contributor

Gather definitions of colors and special characters in log.js.

Closes: #1414

Gather definitions of colors and special characters in log.js.

Closes: GoogleChrome#1414
Copy link
Collaborator

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

just a question about constants

}

Log.green = '\x1B[32m';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason why you don't make this constants? They don't need to be on the class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having them in class doesn't require excessive import statements like import {green, red, bold ... . I don't mind changing this if you prefer named exports for each of those.

Copy link
Collaborator

Choose a reason for hiding this comment

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

no not exports :)

but not my call to make so waiting for the rest of the team to give final feedback

const red = '\x1B[31m';
const reset = '\x1B[0m';

class Log {
...
static redify(str) {
  return `${red}${str}${reset}`;
}
module.exports = Log;

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 for @wardpeet's approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that would be the best, but definitions (of tick, yellow and other) are used outside this module for example in print.ts so I guess it has to go into export somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

btw. that was the whole point of this issue: to cut duplications.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, indeed that's a fair point.

So, it's the decision of either class or exports.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea of using language constructs (static getters).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ebidel, right something like:

class Log {
   static get green() { return '\x1B[32m'; }
}

I'll send a commit.

@XhmikosR
Copy link
Contributor

XhmikosR commented Jan 8, 2017

@Lokson: thanks for this, I had started doing it locally so this saved me some time :)

@maciej-ka
Copy link
Contributor Author

@XhmikosR: welcome!

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

LGTM, though others can weigh in on inflicting my greenify() and redify() functions on code outside of smokehouse :)

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

great stuff. :)

@paulirish paulirish merged commit 29cffe9 into GoogleChrome:master Jan 9, 2017
@paulirish
Copy link
Member

👩‍🎨🎨

@maciej-ka maciej-ka deleted the refactor-output branch January 10, 2017 03:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants