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

Code styling #349

Closed
8 of 9 tasks
troyanskiy opened this issue May 4, 2018 · 21 comments
Closed
8 of 9 tasks

Code styling #349

troyanskiy opened this issue May 4, 2018 · 21 comments

Comments

@troyanskiy
Copy link
Contributor

troyanskiy commented May 4, 2018

Hello @Romakita

I would like to propose to change code style

I propose following:

  1. Use single quote instead of double quote
  2. 2 space for indentation code, instead 4 space
  3. No unused variables to add to tslint
  4. All interfaces should start with I. add tslint.
  5. Always use const instead of let if the variable is not reassigned
  6. Do not use any if the type can be specified, can have exceptions
  7. Type-assertion <type>varName -> varName as type
  8. Use only arrow functions, can have exceptions if really need function format
  9. A variable name should not start from the capital. Can have exceptions if the variable is constant
  10. Array types. Array<T> -> T[]
  11. Object literal shorthand {format: format} -> {format}
  12. New new MyObject -> new MyObject()

The list is not finished.

Just want to discuss that and I can do some refactoring after.

Thanks!

Just put that decision on the top:

Use single quote instead of double quote
2 space for indentation code, instead 4 space
All interfaces should start with I. add tslint. Issue #350

  • No unused variables to add to tslint
  • Always use const instead of let if the variable is not reassigned
  • New line berfore return
  • Do not use any if the type can be specified, can have exceptions
  • Type-assertion varName -> varName as type
  • Use only arrow functions, can have exceptions if really need function format
  • A variable name should not start from the capital. Can have exceptions if the variable is constant
  • Array types. Array -> T[]
  • Object literal shorthand {format: format} -> {format}
@Romakita
Copy link
Collaborator

Romakita commented May 5, 2018

Hello @troyanskiy,

1 and 2. I follow the official guideline https://github.com/Microsoft/TypeScript/wiki/Coding-guidelines
Double quote and 4 spaces are recommended by Microsoft, also, default tslint configuration follow this rules too. I know, many of you (@milewski ^^) want use the single quote and 2 spaces in the code XD. I use this convention with eslint.

I propose a vote for that ;).

  1. No variable unused, I'm OK :)
  2. My bad, I started with this convention but Microsoft recommend "Do not use "I" as a prefix for interface names.", so I started to change that with the v2. Currently, all interfaces name isn't normalized. Now it's big change, but if you want work on, I agree ;)
  3. This is already the case, no ?
  4. Currently, the notImplicitAny is enable, and in almost case, I try to used the type annotation. I agree with you for this rule :)
  5. varName as type, ok :)
  6. I agree. But for exported function, it's preferable to export a function instead of arrow function. The function names are better analyzed by IDEs.
  7. Totally agree, if you have found a variable that does not follow this convention, it must be fixed. Rule can be added too.
  8. T[] ok :)
  9. Ok, I already use that when i's possible :)
  10. Totally agree.

Thanks for your proposals, I'm happy about that ;)

See you,
Romain

@Romakita
Copy link
Collaborator

Romakita commented May 5, 2018

@troyanskiy You can work on the rules which are simple to set up in a first and on which we are ok :)

@milewski
Copy link
Contributor

milewski commented May 5, 2018

  1. Cant agree more
  2. 4 spaces please
  3. Sure
  4. I completely disagree with this one :( long ago the official recommendation from MS was do never user I for interface they ended up removing this recommendation due to fight with some users coming from other languages where I is kind of standard for interface: example: Prohibition against prefixing interfaces with "I" microsoft/TypeScript-Handbook#121, but i kind agree not using I because nothing starting with I l (iL) looks pretty anyways....
  5. yes
  6. yes
  7. <type>varName ts team discourage this way.. it is legacy and i forgot where in their doc i read it to stop using it
  8. agree with Romakita
  9. agree
  10. this T[] it also a official recommendation
  11. agree {format}
  12. agree new MyObject()

@Romakita
Copy link
Collaborator

Romakita commented May 5, 2018

Yes for the "I" it's a problem. I started with, but now some interfaces doesn't follow this rules. Now, we can follow this rules and prepare the code migration to this goal. I can create an Issue about that.
Also, we can add a guideline section in the contributions.md about the code style adopted by us :)

@milewski
Copy link
Contributor

milewski commented May 5, 2018

in my daily life the convention i adopt for interfaces as well as other codding patterns are the one usually used within the php community,
SomethingInterface, SomethingAbstract, SomethingRepository, SomethingFactory, SomethingRegistry so on....

@Romakita
Copy link
Collaborator

Romakita commented May 5, 2018

Story was created here #350

@Romakita Romakita added this to the BACKLOG milestone May 5, 2018
@troyanskiy
Copy link
Contributor Author

Ok. So. I will create a branch for each subrefacoring task and will start some refactoring.

Or how it’s better to do?

PS: Personally i don’t agree with some MS recommendation. Aspacially about double quote and 4 spaces :)

@Romakita
Copy link
Collaborator

Romakita commented May 5, 2018

Yes a branch for each subrefactoring task is a better solution ;)

I know, this is why you have created this topics :p

@troyanskiy
Copy link
Contributor Author

Hahaha @Romakita totally right ;)

@milewski
Copy link
Contributor

milewski commented May 5, 2018

or perhaps we could use prettier https://github.com/prettier/prettier \o/ i dont agree with some of their rules but in the end in the team i work on everything averages out... besides it has been mass adopted by many big open source projects out there

@troyanskiy
Copy link
Contributor Author

troyanskiy commented May 5, 2018

So.
To summarize

Moved to the first comment of the issue

@troyanskiy
Copy link
Contributor Author

troyanskiy commented May 5, 2018

Add to coding styles one more rule
Kind of new line before return;

public set(target: Type<any> | symbol, provider: I) {
        this.registry.merge(target, provider);
        return this;
    }

Should be like that

public set(target: Type<any> | symbol, provider: I) {
        this.registry.merge(target, provider);

        return this;
    }

Please vote for that with likes or dislikes

@troyanskiy
Copy link
Contributor Author

troyanskiy commented May 5, 2018

No used variable.

Changes have been done in https://github.com/Romakita/ts-express-decorators/tree/style-no-unused-variables branch.

@Romakita Could you please check my commit comments plz?

Thanks!

@Romakita
Copy link
Collaborator

Romakita commented May 6, 2018

@milewski Why not, prettier could be a nice solution :).

Who work on this ?

@milewski
Copy link
Contributor

milewski commented May 6, 2018

If you are all in favor of prettier I can do it... :)

@troyanskiy
Copy link
Contributor Author

@milewski I'm totally for that if it will replace all double quotes to single and replaces all ident with 2 spaces... :D
At least you can play with it and see the result.

@troyanskiy
Copy link
Contributor Author

I have added to line new line before return.

@troyanskiy
Copy link
Contributor Author

troyanskiy commented May 6, 2018

@Romakita Romakita modified the milestones: BACKLOG, SPRINT MAI May 7, 2018
@Romakita
Copy link
Collaborator

Romakita commented May 7, 2018

@milewski You can work on prettier ;)

@Romakita
Copy link
Collaborator

Romakita commented May 17, 2018

Hi all,

There remains only this:

  • Do not use any if the type can be specified, can have exceptions

Otherwise, prettier work fine :)

Good works guys !
See you

@troyanskiy
Copy link
Contributor Author

@Romakita I think we can close the issue.
It's really hard to put now the rule to tslint no to use any because it will be tonnes of errors at linting.

@Romakita Romakita modified the milestones: SPRINT MAI, BACKLOG Jul 3, 2018
@Romakita Romakita closed this as completed Jul 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants