Support disallowing namespaces (noNamespace).

[mprobst]

This allows code bases to outlaw pre-ES6 modules and namespaces. It's often
still useful to declare namespaces when interacting with non-TypeScript code, so
this has an option to allow "declare namespace" style usage.

[jkillian]

Can we give this class a more meaningful name, like NoNamespaceWalker?

[mprobst]

Done.

[jkillian]

Thanks @mprobst, I like this rule! Makes sense to me to have this rule as more TS codebases will want to enforce ES6 module usage over namespaces.

Left you a few tweaks to make, and then we should be able to merge this. Looks like the rule also needs a rebase, a previously merged PR must've created a conflict

[mprobst]

Thanks for the review! Please take another look.

[mprobst]

So, in the mean time I found the no-internal-module rule (no idea how I didn't notice that before). I think it might make sense to unify the two configuration settings - no-namespace is sort of a natural extension of no-internal-module, and the code should share the logic around global augmentations and nested declarations.

I've added that as a separate commit as it makes the configuration a bit harder to understand. Let me know what you think.

[jkillian]

It's a tough call. In my mind:

no-internal-module encourages the use of the namespace keyword instead of the outdated module keyword where appropriate

no-namespace encourages people to stop using namespaces all together and instead organize their code with ES6 modules.

They're definitely about similar syntaxic elements, but the goals of the two are somewhat different. I'm unsure here about what's best.

[mprobst]

I agree, it's a bit tricky, and I presume it might also be nicer to have just one option going forward (no-namespace). I could also refactor the code so we only have one shared implementation, but still two different options.

How do we resolve? Get a third opinion?

[jkillian]

@adidahiya any thoughts?

[mprobst]

@alexeagle knows his way around static analysis tools and might venture an opinion?

[alexeagle]

I think tslint lacks a curated default set of checks, in which case things that have been deprecated by the language (no-internal-module) should be enabled by default and users don't think about it (unless they need to disable it). If that were the case, the answer would clearly be to make a new, separate no-namespace rule.

In my opinion, the convenience of no-namespace implying no-internal-module is not worth the complexity of understanding the configuration. Are there any other options that currently flip other options? Users are expected to study a long list of rules and decide what to opt-in to, so follow that model.

[jkillian]

Makes sense to me. We should be sure that no-namespace doesn't conflict with no-internal-module though: that is, it shouldn't be possible to get into a position where one of the two rules will always error.

An example of this would be if you had a rule that required explicit typedefs and had a rule that required implicit typedefs. It would be impossible to declare a variable and satisfy both of these rules.

I think we'll be fine here, as no-namespace is just more strict that no-internal-module which shouldn't lead to conflicts. Let's keep it as a separate no-namespace rule if you're in agreement @mprobst

[mprobst]

Agreed, sounds good to me. Please take another look; I've "unmerged" the code, and now just share the implementation of isNestedDeclaration across the two lint rules.

[mprobst]

Friendly ping :-)

[jkillian]

Can you instead move this function to src/language/utils and rename it isNestedModuleDeclaration? Then we avoid any chance of circular dependencies and keep shared code in a separate location

[jkillian]

Sorry for the delay! Can you make the one change listed above and also rebase off the latest develop? (The rebase will help make sure that CI passes)

[mprobst]

No worries! Refactored and rebased, PTAL.

[jkillian]

Looks great, thank you!