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

Reformat the code with best practices using TSLint #8

Open
Leoat12 opened this issue Nov 11, 2018 · 5 comments
Open

Reformat the code with best practices using TSLint #8

Leoat12 opened this issue Nov 11, 2018 · 5 comments

Comments

@Leoat12
Copy link
Owner

@Leoat12 Leoat12 commented Nov 11, 2018

It is always important to have a clean and easy to understand code and in my opinion it is done by using best practices recognized by the community, so my idea is using TSLint to reformat the code based on the its suggestions, which already proved to be good practices.
I already added TSLint to the dev dependencies and I'm working now on reformatting the code.

It is under discussion, though, what rules will be used, so use this issue to discuss changes on the tslint.json file. For example, I added a exception to the recommended style by putting "no-console": false since we use console.info() and console.error() and others a lot. It may change if we find better ways to print on the screen.

When we reach the point of mature on this we can require from pull requests compliance with the TSLint rules through CI tools, I think it is possible to do this. It is for the far future though, it would be way too strict to do this at this point.

@athif23

This comment has been minimized.

Copy link
Contributor

@athif23 athif23 commented Nov 11, 2018

I just found some tool named signale that can be used to print on the screen.

What do you think?

@Leoat12

This comment has been minimized.

Copy link
Owner Author

@Leoat12 Leoat12 commented Nov 12, 2018

I was aware of signale when I studied taskbook code, but we also need to think about the amount of dependencies we already have and if it is worth adding another one. signale has much more features that we can use to beautify the CLI interactiveness. However, if we just want to avoid console.info(), it is better use process.stdout.write(), which is native from nodejs.
To avoid writing this long call every time we can do this on Display:

private static write(message: string){
process.stdout.write(message);
}

What do you think, use signale to refactor the display of the CLI entirely using it or just change console.info() for process.stdout.write()?
I will study signale a little bit more meanwhile.

@athif23

This comment has been minimized.

Copy link
Contributor

@athif23 athif23 commented Nov 12, 2018

I agree, let's not use signale, we already have too many dependencies.
Just want to know, what is the advantage of using process.stdout.write() than console.info()?

@Leoat12

This comment has been minimized.

Copy link
Owner Author

@Leoat12 Leoat12 commented Nov 12, 2018

TSLint says that console methods are not suitable for production. I think it is true for web applications, but not for NodeJS applications, right? Maybe we can stay with console.info() until further notice. Don't you agree?

@athif23

This comment has been minimized.

Copy link
Contributor

@athif23 athif23 commented Nov 12, 2018

Well, if there's nothing really different, let just use the easier one. That's better and it makes the code more readable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.