Skip to content

Add rules about exception types naming #31

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Mar 3, 2018
Merged

Add rules about exception types naming #31

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9686e9a
Add superfluous suffixes sniffes
carusogabriel Feb 28, 2018
82abd7d
Add rules about exception types naming
greg0ire Feb 25, 2018
92ccb8a
Document forbidden suffixes and prefixes
greg0ire Feb 28, 2018
9af0a8c
Require ^4.5.0
Mar 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ exceptions/differences/extensions (:white_check_mark: are the implemented sniffs

- Keep the nesting of control structures per method as small as possible
- Prefer early exit over nesting conditions or using else
- :white_check_mark: Abstract classes should not be prefixed with `Abstract`
- :white_check_mark: Interfaces should not be suffixed with `Interface`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See https://github.com/doctrine/coding-standard/pull/31/files#r171371131

- :white_check_mark: Concrete exception class names should not be suffixed with `Exception`
- :white_check_mark: Abstract exception class names and exception interface names should be suffixed with `Exception`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @kukulich said, this isn’t possible 😞

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not? You can assume inheritance - when ancestor has the Exception suffix, this class has too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right but:

  • Not all exceptions has Exception suffix, eg. TypeError.
  • You cannot report all bugs in one run with code like A extends B extends C extends Exception. You can report C in first run and B in second.

So if you're ok with this, it's possible to check "Abstract exception class names". However, I think it's really not possible to report "exception interface names should be suffixed with Exception"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove the check mark on that one then.

Copy link
Member Author

@greg0ire greg0ire Mar 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh I was too slow, @Ocramius just merged :P

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@greg0ire fixup patch with discussion there please :P

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay

- :white_check_mark: Align equals (`=`) signs in assignments
- :white_check_mark: Add spaces around a concatenation operator `$foo = 'Hello ' . 'World!';`
- :white_check_mark: Add spaces between assignment, control and return statements
Expand Down
2 changes: 1 addition & 1 deletion composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
"php": "^7.1",
"squizlabs/php_codesniffer": "^3.2",
"dealerdirect/phpcodesniffer-composer-installer": "^0.4.2",
"slevomat/coding-standard": "^4.4.0"
"slevomat/coding-standard": "^4.5.0"
},
"extra": {
"branch-alias": {
Expand Down
6 changes: 6 additions & 0 deletions lib/Doctrine/ruleset.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,12 @@
</rule>
<!-- Forbid dead code -->
<rule ref="SlevomatCodingStandard.Classes.UnusedPrivateElements"/>
<!-- Forbid suffix "Abstract" for abstract classes -->
<rule ref="SlevomatCodingStandard.Classes.SuperfluousAbstractClassNaming"/>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should lines 97 and 101 be moved to other PRs? Or should I add them to the standard?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I add all of them ‘cause of #31 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok let me document that too then.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SuperfluousAbstractClassNaming and SuperfluousInterfaceNaming forbid both suffix and prefix. So you should document this or add <exclude> to ignore the prefix check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@greg0ire I guess we want just the suffix here, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aaah ok yes, I think that's it, because I saw a lot of Abstract prefixes in the code.

<!-- Forbid suffix "Exception" for exception classes -->
<rule ref="SlevomatCodingStandard.Classes.SuperfluousExceptionNaming"/>
<!-- Forbid suffix "Interface" for interfaces -->
<rule ref="SlevomatCodingStandard.Classes.SuperfluousInterfaceNaming"/>
<!-- Forbid useless annotations - Git and LICENCE file provide more accurate information -->
<rule ref="SlevomatCodingStandard.Commenting.ForbiddenAnnotations">
<properties>
Expand Down