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

Consistent rules naming #57846

Closed
6 tasks done
medreres opened this issue Mar 19, 2024 · 10 comments
Closed
6 tasks done

Consistent rules naming #57846

medreres opened this issue Mar 19, 2024 · 10 comments
Labels
Not a Defect This behavior is one of several equally-correct options

Comments

@medreres
Copy link

medreres commented Mar 19, 2024

πŸ” Search Terms

compilerOptions, allowUnreachableCode, allowUnusedLabels, noStrictGenericChecks

βœ… Viability Checklist

⭐ Suggestion

The naming convention is not consistent across rules in compiler options, therefore obstructing intuitive and straightforward code styling.
One bulk of compiler rules goes with "allow" prefix, another goes with "no" and there is third group using "suppress".
I suggest unifying those rules. It will reduce the need of consulting the documentation and will provide more clarity to the codebase.
Rules with old naming could be marked as deprecated.

πŸ“ƒ Motivating Example

This

{
  "compilerOptions": {
    "allowUnreachableCode": false,
    "allowUnusedLabels": false,
    
    "noImplicitUseStrict": false,
    "noStrictGenericChecks": false,
  }
}

versus this

{
  "compilerOptions": {
    "noUnreachableCode": false,
    "noUnusedLabels": false,
    
    "noImplicitUseStrict": false,
    "noStrictGenericChecks": false,
  }
}

πŸ’» Use Cases

  1. What do you want to use this for?
    For easier learning of details of ts config and easier setup of projects
  2. What shortcomings exist with current approaches?
    No shortcomings, the need of consulting the docs will be reduced to minimum
  3. What workarounds are you using in the meantime?
    Remembering the options and consulting the docs.
@MartinJohns
Copy link
Contributor

Adding even more options (or renaming them) will only bring more confusion, and you still need to consult the documentation to understand what the options do.

I've seen similar issues already, and they're always rejected. There gotta be a really good reason for such a change, and IMO you haven't presented any.

therefore obstructing intuitive and straightforward code styling.

It's not code, it's a configuration file.

@RyanCavanaugh RyanCavanaugh added the Not a Defect This behavior is one of several equally-correct options label Mar 19, 2024
@RyanCavanaugh
Copy link
Member

I don't really see what the upside is here. There's already a completion list here; if you type "returns" you will see that it's noImplicitReturns not allowImplicitReturns.

Conversely, if two options that control the same thing exist:

  • You have to figure out what it means if both exist
  • You have to figure out what it means if both exist in different config files
  • Basic string search no longer works
  • Someone will log an issue saying you have to remove one of them "for consistency"
    • Then everyone argues about which one is correct
  • Someone else logs an issue saying that all options should be phrased in a way that the default value of false is correctly interpeted "for consistency"
  • Someone else logs an issue saying that they should not be renamed since the options in 5.5 were obviously good enough, and having two names is bad "for consistency"

@RyanCavanaugh RyanCavanaugh closed this as not planned Won't fix, can't repro, duplicate, stale Mar 19, 2024
@fatcerberus
Copy link

Let's just remove all the options and have tsc behave the same for everyone and be completely nonconfigurable... "for consistency" 🚎

@medreres
Copy link
Author

@fatcerberus let's just be more friendly eh?

@medreres
Copy link
Author

medreres commented Mar 20, 2024

@RyanCavanaugh it's just I was studying ts config docs and it appeared to me counterintuitive to have such namings for rules in the first place. You made good point about this as well.

Someone else logs an issue saying that all options should be phrased in a way that the default value of false is correctly interpeted "for consistency"

Those are small things and are easily looked up in docs, but throughout time they gather like snowball and in the end you just have to consult docs for every such details and it's not a good developer experience IMHO.

Or maybe there is some specific reason why those rules were named like this? Or this is just a simple human error?

@fatcerberus
Copy link

fatcerberus commented Mar 20, 2024

let's just be more friendly eh?

@medreres That's the reason for the 🚎 emoji - trolleybus for friendly/playful trolling (and in this case I was trolling Ryan, not you). Don't take it too seriously

Those are small things and are easily looked up in docs, but throughout time they gather like snowball and in the end you just have to consult docs for every such details and it's not a good developer experience IMHO.

I would argue if you care about the options in a tsconfig at all you should be consulting the docs anyway, regardless of what they're named. The name alone can't convey all the effects a given option has, particularly in combination with other options, so what things are called is the least of your worries.

Or maybe there is some specific reason why those rules were named like this? Or this is just a simple human error?

More like "organic growth over time"; individual options are named pragmatically as they're introduced, naming preferences change, etc. etc. There's no specific reason for the haphazard naming beyond "that's just the way it worked out".

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Mar 20, 2024

Or maybe there is some specific reason why those rules were named like this?

They're named such that their default values are all false (or at least were at the time when they were named, though I don't think any have changed yet)

For example, if the default behavior is that foo is not allowed, then the flag is called allowFoo

If the default behavior is that bar is allowed, then the flag is called noBar

This keeps you from having to look up in the docs what the default value is - by looking at the name, you can tell.

Edit: The only flag that disobeys this rule is forceConsistentCasingInFileNames, which was false by default for a long time because there were performance problems associated with Node's syscalls for checking it. Once those were fixed, we changed it to true; its "proper" name would now be alllowInconsistentCasingInFileNames but this flag is so rarely set that it's really not worth introducing a second name for or introducing a config break.

@RyanCavanaugh
Copy link
Member

Note that this scheme is way better in terms of "saving you a trip to the docs" -- the completion list of options itself encodes their default values via the naming convention.

I think it'd be a lot worse if you were looking at a list of allowUmdGlobalAccess, allowPropertyAccessFromIndexSignature, allowImplicitOverride, allowUncheckedIndexedAccess, saying to yourself "Want that, don't want that, want that, don't want that" and having to cross-reference with the docs to see if you need to explicitly toggle them or not.

@medreres
Copy link
Author

This keeps you from having to look up in the docs what the default value is - by looking at the name, you can tell.

Interesting thing, good to know, thanks :)

Didn't get the second point about self encoding naming though

@fatcerberus
Copy link

The self-encoding naming point is just a restatement of the bit you quoted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Not a Defect This behavior is one of several equally-correct options
Projects
None yet
Development

No branches or pull requests

4 participants