Decide a coding style guide for the project

[RobPavey]

It is important to at least agree on an indentation style and a brace layout style.

[RobPavey]

I would recommend choosing an existing style rather than inventing a new one.

I would recommend Google's style: https://google.github.io/styleguide/jsguide.html

[PeWu]

In my projects I use what prettier provides out of the box. The upside is that you just run the tool, it formats your code and you don't have to think about style anymore.

[jmegenealogy]

I also use prettier (although it did something weird to the table in https://raw.githubusercontent.com/wikitree/wikitree-browser-extension/development/README.md).

It looks like Google's style includes other useful things like naming conventions and JSDoc stuff, so we may want to go with that.

[ke4tch]

Most of the Google style items look good to me. Just reviewing that has pointed out some things for me to clean up as a JavaScript newbie.

[ke4tch]

Here are some other coding style guidelines to consider:

https://www.w3schools.com/js/js_conventions.asp

https://developer.mozilla.org/en-US/docs/MDN/Writing_guidelines/Writing_style_guide/Code_style_guide/JavaScript

The former is short and sweet, the latter similar to the Google guidelines without Google specific features.

[shogenapps]

Although I hadn't seen the Google standards until Rob posted a link to them recently, they seem like a very good standard to follow, and I will try to follow that. And I've just discovered Prettier, thanks to the comments above, so that will help.

[RobPavey]

We have just tried out prettier on the Sourcer project and the defaults look OK to me other than the default line wrap which is 80 I believe. I suggest changing that to 120. e.g.: "printWidth": 120,

[bcaseyrls]

I suggest we use the same .prettierrc configuration file for both wikitree-dynamic-tree and wikitree-browser-extension. The dynamic tree repo has a (draft) .prettierrc file (https://github.com/wikitree/wikitree-dynamic-tree/blob/main/.prettierrc) which sets the line length to 120. See https://github.com/wikitree/wikitree-dynamic-tree/issues/31 and https://github.com/wikitree/wikitree-dynamic-tree/pull/34.

[RobPavey]

I'm not sure how that prettierrc differs from the defaults. I would just go with the defaults apart from the line length.

[ke4tch]

Does it make sense to have a prettier webpack target so that you can npm run prettier as you would for npm run lint

[RobPavey]

Or it could be setup to run on commit also.

[MichalVasut]

Prettier has the option to run from command line and overall it integrates very well. ;-)

[jmegenealogy]

Or it could be setup to run on commit also.

Just putting this here for later: https://mskelton.dev/blog/auto-formatting-code-using-prettier-and-github-actions

[PeWu]

Prettier can also be run with GitHub Actions in the form of a test (prettier --check). If the code is not the same as run through prettier, the PR will have a check failure.

In terms of style, my preference is to stick with whatever is the default. If prettier is used, use its defaults. Arguing about style is quite counterproductive. Everyone has different preferences. Sticking to defaults is always the least controversial even though there is someone that made that decision.

[shogenapps]

Let's use the .prettierrc file that is being used on the dynamic tree side of things, as Brian suggested 13 days ago. Everyone may have their preferences, but someone (maybe Brian?) has made a decision about standard formatting. Why don't we just go with that?

[RobPavey]

Let's use the .prettierrc file that is being used on the dynamic tree side of things, as Brian suggested 13 days ago. Everyone may have their preferences, but someone (maybe Brian?) has made a decision about standard formatting. Why don't we just go with that?

It would be nice if there was some rationale for the rules we choose. Like a vote among the the current contributors. Or looking at what the general standard is. As Kay pointed out in Discord, most of the coding guidelines for js use an indentation of 2.

The following all say 2 spaces:

• https://javascript.info/coding-style
• https://www.w3schools.com/js/js_conventions.asp
• https://developer.mozilla.org/en-US/docs/MDN/Writing_guidelines/Writing_style_guide/Code_style_guide/JavaScript
• https://google.github.io/styleguide/jsguide.html

[shogenapps]

On second thoughts, maybe some people here don't want to use the same file as the dynamic tree people. I'll do anything. If most people here want Prettier defaults and printWidth of 120, I'll do that. I'm going to do that now until someone tells me otherwise.

[shogenapps]

Let's use the .prettierrc file that is being used on the dynamic tree side of things, as Brian suggested 13 days ago. Everyone may have their preferences, but someone (maybe Brian?) has made a decision about standard formatting. Why don't we just go with that?

It would be nice if there was some rationale for the rules we choose. Like a vote among the the current contributors. Or looking at what the general standard is. As Kay pointed out in Discord, most of the coding guidelines for js use an indentation of 2.

I read the Google style rules you posted recently and they had a 2 space indentation. If it's good enough for Google...

[MichalVasut]

As for me, I don't really care if it'll be 80 or 120 charcters (120 works for us at work, but it was about the agreement in team).

As for 2 vs 4 - it's same as 1st point, I don't really care - again I'm from Python used to 4 characters, but JS has probably different standards and it's OK.

The main point, why I wanted to make some rules (and even enforce them e.g. via Github Actions) in Dynamic Tree is because it was really messy and distracting when comparing versions in merge requests.

Btw, today, U've read interesting rationale why Python decided for 80 characters:

https://discuss.python.org/t/maximum-line-length-restriction/13246/3

[RobPavey]

As for me, I don't really care if it'll be 80 or 120 charcters (120 works for us at work, but it was about the agreement in team).

I'm sure I can get used to whatever is agreed but the 80 character printWidth is rather jarring to me at the moment. The last place I worked had guidelines against using abbreviations and encouraged more verbose function and variable names. So a function call with a few parameters that is indented a few levels gets switched from one line to five lines by prettier when using printWidth of 80.

Example this:

function fillDefaultValuesForOptions(defaultValues, options, useTestDefaults, sync) {
  for (let option of options) {
    if (option.type == OptionType.GROUP) {
      if (option.options) {
        fillDefaultValuesForOptions(defaultValues, option.options, useTestDefaults, sync);
      }
    }
  } 
}

becomes this:

function fillDefaultValuesForOptions(
  defaultValues,
  options,
  useTestDefaults,
  sync
) {
  for (let option of options) {
    if (option.type == OptionType.GROUP) {
      if (option.options) {
        fillDefaultValuesForOptions(
          defaultValues,
          option.options,
          useTestDefaults,
          sync
        );
      }
    }
  } 
}

Just providing that hypothetical example so people can judge which they find more readable.

I wonder if a printWidth of 80 would encourage me to abbreviate names more.

Like I say, I'm sure I can get used to it if that is what people prefer.

[RobPavey]

I will put up a PR with a .prettierrc and we can decide the issue there

[MichalVasut]

@RobPavey

I will put up a PR with a .prettierrc and we can decide the issue there

we already have one here: https://github.com/wikitree/wikitree-dynamic-tree/blob/main/.prettierrc

feel free to use it

[ke4tch]

So could the .prettierrc become a test case for files that need to be in both the dynamic tree and the browser?

[RobPavey]

So could the .prettierrc become a test case for files that need to be in both the dynamic tree and the browser?

Could you elaborate Kay? I'm not sure what you mean.

[ke4tch]

We have one single .prettierrc file that we want to include in the build for both the dynamic tree and the browser. So how can we manage this from a single baseline.

An example, right now I have files duplicated between the browser and the BioCheck app. If I can ever get out of my own coding way, there may be some of those same files that also go in the dynamic tree.

[RobPavey]

We have one single .prettierrc file that we want to include in the build for both the dynamic tree and the browser. So how can we manage this from a single baseline.

I guess the question is: does it matter if we have different .prettierrc rules in wikitree-browser-extension and wikitree-dynamic-tree? If we are doing auto-format on commit (or save or something like that) then if you copy a file from one project to the other it will just be auto-formatted to match the style of the project.

[harrislineage]

I'm sure I can get used to whatever is agreed but the 80 character printWidth is rather jarring to me at the moment.

100% agree with @RobPavey here. I would vote for 120 printWidth, but can adapt to 80 if that is the consensus.

[shogenapps]

Done.