faithlife / codingguidelines Goto Github PK

https://docs.microsoft.com/en-us/dotnet/csharp/whats-new/csharp-10#global-using-directives
https://docs.microsoft.com/en-us/dotnet/core/compatibility/sdk/6.0/implicit-namespaces-rc1

The default C# coding style does not use a space after a cast (example).

We have traditionally used a space after a cast (example).

Few projects on public GitHub set csharp_space_after_cast to true like we do.

Should we stop using a space after a cast like everybody else? Indicate your preference with your reaction:

• 👍 stop using a space after a cast
• 👎 continue to use a space after a cast
• 👀 don't care or aren't sure

Try to sway your coworkers in the discussion below.

Our internal wiki contains a good amount of useful coding guidelines. It also contains obsolete guidelines, specific style guidelines that are now enforced through tools, and project specific guidelines.

Import the generally applicable guidelines from the wiki so that all our current guidelines are in one location (this repo).

https://github.com/Faithlife/CodingGuidelines/blob/master/sections/javascript.md#reactlazy

There can be other reasons to need a default export, for instance Next.js pages must use default exports for the file-system routing.

In the .gitattributes guidelines document, it's suggested to set blame.ignoreRevsFile globally:

• If you haven’t already, ensure that git blame ignores the commit IDs in the file:
  • git config --global blame.ignoreRevsFile .git-blame-ignore-revs

However, if this is set globally, then git blame will fail in any repository that doesn't contain the referenced file:

fatal: could not open object name list: .git-blame-ignore-revs

Considering this, it's probably better that this option be configured on a project-by-project basis, rather than system-wide. Thoughts?

Would it make sense to disable SA1307 if we're going to continue with the old naming convention for static fields? Rider is upset that public static readonly string[] s_searchFields = { "name" }; doesn't begin with an uppercase letter.

As currently configured, the eqeqeq rule forbids using == for null checks. In my experience, however, forbidding this check often leads to authoring of more error-prone code.

First, let's establish the fact that null and undefined are two different values which in virtually all cases mean the same thing for the author, i.e. the value is not set, and attempting to access any properties of the value would throw. And further, any API which behaves differently when given null vs undefined is surprising, and typically frustrating. A function which has handling for undefined, but throws for null, or worse, does something subtly weird like coercing null to the string "null" is surprising. And in the latter case, is difficult to debug.

Given the above, it's reasonable to conclude that any null/undefined check should check for both values, or else be a potential point for bugs, and thereby pain for users and future devs alike.

Javascript has a built-in operator for "is null or undefined", spelled == null. But what are our alternatives, given that it's currently disallowed?

One alternative is a falsy check. Falsiness, however, is quite a bug-prone way to check for null. 0, '', NaN, false, and document.all (bizarrely, due to legacy backward compat reasons) are all also falsy values. I have personally been bitten numerous times in the past by falsy checks triggering behaviors not intended for values of 0 etc, and make it a point of personal policy to avoid using falsiness checks in place of null checks.

Further, the purpose of eqeqeq is to use strict checks, and so using the even-looser falsiness check seems to go against the spirit of the rule.

The other alternative is to always triple-equals check for both null and undefined. Aside from being more verbose (especially when checking properties nested within objects), this also introduces a new surface area for errors.

Firstly, an author may only check null, and not check both. There are several reasons they may do this: perhaps laziness, or forgetfulness, or simply not knowing they need to (which will often be the case for developers new to javascript). Additionally, they may even just opt for the falsiness check to avoid the verbosity of the full check. I have seen both of these approaches taken, and lead to bugs.

Additionally, combining the two checks is a surface area for bugs. DeMorgan's laws must be followed; the isNull check needs to use ||, but the notIsNull check needs to use &&. Any combination of tiredness, forgetfullness, rustiness, or green-ness could lead to accidentally writing the wrong check, or inverting the logic and not remembering to change the boolean operator.

Now, I'm not necessarily arguing that that last error is common, but it is an error surface that simply doesn't exist when using == null, and we all make mistakes from time to time.

So, with the above established, my argument boils mainly down to this: enforcing that null-checks use == null (and that === null is disallowed) helps developers fall into the pit of success. Unfortunately, there's no eslint rule for "prevent using falsiness checks for null checks" (as that would require type information), but we can at least make checking for both null and undefined easy to do, and less bug-prone.

originally posted here

I don't see a justification for why react/jsx-no-literals is enabled. My guess is it's to encourage developers to put strings in localizable bundles. It doesn't, though, it just encourages developers to wrap literals in {''}. Many of our projects override the rule to off. I propose we remove the rule from eslint-config-faithlife.

https://github.com/Faithlife/CodingGuidelines/blob/master/sections/javascript.md#use-default-exports

I'm planning on avoiding default exports in the next project I start. There are several problems with default exports that named exports avoids:

1. Renaming a default exported type silently causes the declaration and the import names to diverge. With named exports, you'll get an error if you fail to update the import sites.
2. It's possible to import a type without ever saying its declaration name; this can make it difficult to find references to the type. With named exports, even if you alias it, you still need to include the exported name.
3. With default exports it's possible to accidentally bind the import name to the wrong thing; it will just silently continue to work until you misuse the API of the actually imported object. With named exports, it is an error to import a name that doesn't exist in the export source.

This article by the author of ESLint about why he's stopped using default exports is well worth a read: https://humanwhocodes.com/blog/2019/01/stop-using-default-exports-javascript-module/

Yes, default exports encourage the practice of one export per file, which should be encouraged--especially for React components; but if we're making a guideline, I'd rather make a guideline specifically for that practice than use default exports as a proxy for it.

We should consider adding IDE0005: Remove unnecessary import to the C# .editorconfig to remove unnecessary using directives.

The default C# coding style indents switch statement case labels (example).

We have traditionally not indented switch statement case labels (example).

Few projects on public GitHub set csharp_indent_switch_labels to false like we do.

Should we start indenting our switch labels like everybody else? Indicate your preference with your reaction:

• 👍 start indenting switch labels
• 👎 continue to not indent switch labels
• 👀 don't care or aren't sure

Try to sway your coworkers in the discussion below.