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

[Codeing Style] for variable description use /** */ comment style over // #3943

Closed
saostad opened this issue Feb 9, 2020 · 11 comments
Closed
Labels
good first issue Good for newcomers

Comments

@saostad
Copy link

saostad commented Feb 9, 2020

in source code I saw variable description in front of each with format //description which is not following JSDocs and also it's not good in VSCode since in mouse pop up over the variable it shows description before variable in format /**description*/ .
if that makes sense it will be the result :
Before:

private rd!: Reader; // Reader provided by caller.
private r = 0; // buf read position.
private w = 0; // buf write position.

After:

/**Reader provided by caller.*/
private rd!: Reader;
/**buf read position.*/
private r = 0;
/**buf write position.*/
private w = 0; 

if you agree I can take care of applying changes in style guide and std modules.

@kitsonk
Copy link
Contributor

kitsonk commented Feb 10, 2020

Only thought:

/**Reader provided by caller.*/

Should be like this:

/** Reader provided by caller. */

@David-Else
Copy link

...and the heading Format code using prettier. is now out of date too.

I really hope the new formatter produce identical output to Prettier, as the intent of Prettier was to create a new standard:

  1. No more debates about what is best, just accept the new standard and get on with life
  2. Diffs will be massively improved as different commits won't change the formatting (providing we all use the Prettier defaults)

@nayeemrmn
Copy link
Collaborator

@David-Else There's no format.ts anymore, but we still use prettier for Deno's source. That should probably change at some point but deno will become a dev dependency of Deno which it isn't already. No problem with that, though.

@saostad
Copy link
Author

saostad commented Feb 10, 2020

so should I consider the discussion as everyone agreed with my proposal?
if yes I will start working on my pull to add it as a rule in style-guide and apply the changes to std modules.

@nayeemrmn
Copy link
Collaborator

Those are private fields... the current style guide is concerned about exported machinery, that's probably intentional. You can change specific comments but the style guide shouldn't enforce anything more IMO.

@kitsonk
Copy link
Contributor

kitsonk commented Feb 10, 2020

I tend to disagree on that. While consuming private fields elsewhere in the class, a properly formed comment is provided in editors consuming the language services. While it certainly is less critical, we should still be consistent.

@saostad
Copy link
Author

saostad commented Feb 11, 2020

I tend to disagree on that. While consuming private fields elsewhere in the class, a properly formed comment is provided in editors consuming the language services. While it certainly is less critical, we should still be consistent.

also if we don't have it in style-guide, eventually it will same again so what's the point of fixing it in the first place?

@nayeemrmn
Copy link
Collaborator

Sorry, I wasn't sure what change is being proposed. I meant that the style guide shouldn't say that private fields require JSDoc since we're already using TS and most things are self-explanatory.

Of course I agree if there is a comment it should be written correctly... that much should be a given though, If not then maybe with small changes in wording.

@saostad
Copy link
Author

saostad commented Feb 12, 2020

ok, I will start work on it

@lucacasonato lucacasonato added the good first issue Good for newcomers label Aug 14, 2020
@Hamish-Bassett
Copy link

Hi all,

Is this still open? I can't see any action after the 14th of Feb so nearly half a year ago at this point.
If @saostad is happy for the assistance, I can help out and work with them on this.

@bartlomieju
Copy link
Member

@Hamish-Bassett thanks, but as far as I can see most of the code adheres to the style guide so this issue is no longer relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

7 participants