Automattic / phpcs-neutron-standard

A set of phpcs sniffs for PHP >7 development
MIT License
94 stars 7 forks source link

Add RequireImportsSniff #57

Closed sirbrillig closed 6 years ago

sirbrillig commented 6 years ago

This sniff requires that all functions, constants, and classes be imported explicitly and marks a warning if an unimported symbol is found.

That is, you cannot just import a namespace or assume that the symbols you use are in your namespace or the global namespace, each dependency must be explicitly imported. This makes dependency trees more clear.

For example:

namespace Vehicles;
use function Vehicles\startCar;
class Car {
  public function drive() {
    startCar();
    goFaster(); // this will be a warning because `goFaster` was not imported
  }
}

Fixes #45

Questions:

sirbrillig commented 6 years ago

@stuwest I'm pleased with how this turned out, but I have a feeling that no one will want to use it, particularly for WordPress. What do you think?

stuwest commented 6 years ago

I agree a lot of WordPress devs won't love it, because we're so used to just winging it with globals.

This makes dependency trees more clear

But I think this is totally worth it.

This feels like a great item as a warning... but likely something that will take a lot of transitioning for existing code bases.

sirbrillig commented 6 years ago

One thing I think that could be problematic here is when the namespace (or part of the namespace) serves to clarify an ambiguous symbol name.

For example, if there was the function Calendar\update() (arguably we should rename the function, but that could be a tall order for existing code bases), this sniff forces it to be called as update(), requiring the developer to look at the top of the file for the context, and possibly conflicting with other names.

I'm going to discuss this further with the Neutron team, but the paths I see going forward might be:

sirbrillig commented 6 years ago

Another issue I thought of is that functions like array_map() require the fully qualified namespace, so they're likely to trigger this warning a lot (added to the PR description). 😩

sirbrillig commented 6 years ago

Closing this in favor of #59 which is simpler, less noisy, and hopefully more helpful.