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

PhpStorm separate profile #65

Merged
merged 3 commits into from
Oct 7, 2024
Merged

PhpStorm separate profile #65

merged 3 commits into from
Oct 7, 2024

Conversation

danepowell
Copy link
Collaborator

Our code standards forbid throws tags since they quickly become stale.

Additionally, I'm testing whether it's better to import the PhpStorm inspections profile as a separate profile rather than clobbering the default one, which might not be nice if folks do more than just Acquia / Drupal development.

@TravisCarden
Copy link
Contributor

Our code standards forbid throws tags since they quickly become stale.

Where is this, @danepowell?

I support making our example PhpStorm profile match our standards, though in the long term I think we should permit @throws tags and encourage the use of PHPStan's exception features to prevent them from getting stale. Particularly these:

parameters:
    exceptions:
        check:
            missingCheckedExceptionInThrows: true
            tooWideThrowType: true

(Someday when we're feeling ambitious, we might consider an Acquia PHPStan extension--and a meta package to tie all of our standards and recommendations together.)

@danepowell
Copy link
Collaborator Author

Oops! This is a slevomat rule in the ACLI standard. I intended to contribute it (along with several others) back to the Acquia Edge standard but it looks like I never did. https://github.com/acquia/cli/blob/9f165fe786a8d20b4dab4adc57ce6cbfa80bec37/phpcs.xml.dist#L40

Anyway... I just think comments, especially function comments, have become way too verbose with so many annotations. When the same info can be communicated by the IDE or native features (e.g., type hints), I prefer to trim the fat.

@TravisCarden
Copy link
Contributor

TravisCarden commented Aug 28, 2023

Ah, I see. Well, I personally would allow @package and @throws (though I would hardly make a fuss of them), but I can definitely support the others. We'll definitely want to check with product teams, though, to make sure they don't depend on any of them.

@danepowell
Copy link
Collaborator Author

Since I created this, I discovered that PhpStorm has a limit on exception discovery (I think it's two levels of method calls), so properly annotating throws is actually pretty important. We discussed this during the recent standards upgrade, I don't have a reference handy.

@danepowell danepowell changed the title PhpStorm profile: disable throws inspection PhpStorm separate profile Oct 7, 2024
@danepowell danepowell marked this pull request as ready for review October 7, 2024 19:46
@danepowell danepowell merged commit 464c660 into develop Oct 7, 2024
20 checks passed
@danepowell danepowell deleted the throws branch October 7, 2024 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants