Add rules about exception types naming #31

greg0ire · 2018-02-25T21:41:54Z

You always know an exception is an exception when catching it or
throwing it, so the Exception suffix is useless, but hard to avoid when
you need to group exceptions together, which is why it should be present
in abstract exception names.
See http://mnapoli.fr/approaching-coding-style-rationally/#the-exception-suffix

This is the first of a series of goals I want to clarify regarding exceptions in Doctrine before trying to make a BC version of doctrine/orm#6743

At the moment, things are of course absolutely not like that, but I think that those rules could apply to newly-created exceptions.

Ocramius

This makes a lot of sense to me 👍

@carusogabriel can you maybe give the implementation a shot?

carusogabriel · 2018-02-26T06:46:38Z

@Ocramius Two things:
First: What is your opinion about Abstract and Interface suffixes? Maybe we can extend this to them as well.

Second: Just me fixing manually them is enough, or should we create/search a new Sniff?

Ocramius · 2018-02-26T07:15:26Z

About the suffix, we don't use it within the doctrine organisation, so we better make that clear. I use it for my own projects, but that's a different story.

We'd need to create a sniff: enforcing by documentation is not effective.

kukulich · 2018-02-26T07:36:28Z

Concrete exception class names should not be suffixed with Exception

The check is possible. The same goes for Abstract and Interface suffixes. However it's not possible to make the sniff fixable.

Abstract exception class names and exception interface names should be suffixed with Exception

This is not possible to check with sniff.

carusogabriel · 2018-02-26T07:36:44Z

@Ocramius Perfect. So I’ll try to create our own Sniff fixing it, or see if there’s an existing one!

@kukulich Can you help us with this one as well? I believe that isn’t difficult, a simple check for class modifiers should report it, right?

kukulich · 2018-02-26T07:55:01Z

@carusogabriel Yes, the sniffs are already in my todo list :) Should be done this week :)

kukulich · 2018-02-27T21:54:07Z

@carusogabriel You can try sniffs in dev-master :)

carusogabriel · 2018-02-27T22:02:10Z

@kukulich Men, you are fast 😱

@greg0ire Do you mind if I add commits to your PR, or do you wanna try it?

greg0ire · 2018-02-27T23:19:37Z

@carusogabriel please do :)

greg0ire · 2018-02-28T18:21:43Z

lib/Doctrine/ruleset.xml

    <!-- Forbid dead code -->
    <rule ref="SlevomatCodingStandard.Classes.UnusedPrivateElements"/>
+    <!-- Forbid suffix "Abstract" for abstract classes -->
+    <rule ref="SlevomatCodingStandard.Classes.SuperfluousAbstractClassNaming"/>


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

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

Ok let me document that too then.

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

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

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

You always know an exception is an exception when catching it or throwing it, so the Exception suffix is useless, but hard to avoid when you need to group exceptions together, which is why it should be present in abstract exception names. See http://mnapoli.fr/approaching-coding-style-rationally/#the-exception-suffix

Majkl578 · 2018-02-28T18:41:29Z

I am still against dropping the Exception suffix though.

carusogabriel · 2018-02-28T18:41:44Z

README.md

+- :white_check_mark: Abstract classes should not be prefixed with `Abstract`
+- :white_check_mark: Interfaces should not be suffixed with `Interface`
+- :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`


As @kukulich said, this isn’t possible 😞

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

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"

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

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

@greg0ire fixup patch with discussion there please :P

kukulich · 2018-02-28T20:13:27Z

README.md

 - 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`


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

carusogabriel · 2018-03-02T16:27:09Z

composer.json

        "squizlabs/php_codesniffer": "^3.2",
        "dealerdirect/phpcodesniffer-composer-installer": "^0.4.2",
-        "slevomat/coding-standard": "^4.4.0"
+        "slevomat/coding-standard": "dev-master"


Version 4.5.0 was just released :)

We need the new sniffs.

Ocramius · 2018-03-03T11:03:03Z

I am still against dropping the Exception suffix though.

@Majkl578 let's give it a shot - I think @greg0ire made awesome progress on the ORM exceptions, so we will see if no suffix leads to confusion :)

Ocramius · 2018-03-03T11:05:43Z

🚢-ing this as-is. If it causes problems, we can revert it, but it is worth trying new naming conventions while we still can 👍

Ocramius approved these changes Feb 26, 2018

View reviewed changes

carusogabriel added the New sniff label Feb 26, 2018

carusogabriel added this to the 4.0.0 milestone Feb 26, 2018

carusogabriel force-pushed the exception_suffix branch 2 times, most recently from 0fe5e6c to 9cf56ce Compare February 28, 2018 09:10

carusogabriel added Improvement WIP labels Feb 28, 2018

greg0ire force-pushed the exception_suffix branch from 9cf56ce to 86b9b4b Compare February 28, 2018 18:11

greg0ire commented Feb 28, 2018

View reviewed changes

carusogabriel and others added 2 commits February 28, 2018 19:23

Add superfluous suffixes sniffes

9686e9a

greg0ire force-pushed the exception_suffix branch from 86b9b4b to 82abd7d Compare February 28, 2018 18:23

Document forbidden suffixes and prefixes

92ccb8a

carusogabriel suggested changes Feb 28, 2018

View reviewed changes

kukulich reviewed Feb 28, 2018

View reviewed changes

carusogabriel reviewed Mar 2, 2018

View reviewed changes

Require ^4.5.0

9af0a8c

We need the new sniffs.

Ocramius self-assigned this Mar 3, 2018

Ocramius removed the WIP label Mar 3, 2018

Ocramius merged commit b15293e into doctrine:master Mar 3, 2018

greg0ire deleted the exception_suffix branch March 3, 2018 11:06

greg0ire mentioned this pull request Mar 5, 2018

Forbid Abstract, Exception and Interface prefixing too #38

Merged

Add rules about exception types naming #31

Add rules about exception types naming #31

Uh oh!

Conversation

greg0ire commented Feb 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ocramius left a comment

Choose a reason for hiding this comment

Uh oh!

carusogabriel commented Feb 26, 2018

Uh oh!

Ocramius commented Feb 26, 2018

Uh oh!

kukulich commented Feb 26, 2018

Uh oh!

carusogabriel commented Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kukulich commented Feb 26, 2018

Uh oh!

kukulich commented Feb 27, 2018

Uh oh!

carusogabriel commented Feb 27, 2018

Uh oh!

greg0ire commented Feb 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Majkl578 commented Feb 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire Mar 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ocramius commented Mar 3, 2018

Uh oh!

Ocramius commented Mar 3, 2018

Uh oh!

Uh oh!

greg0ire commented Feb 25, 2018 •

edited

Loading

carusogabriel commented Feb 26, 2018 •

edited

Loading

greg0ire Mar 3, 2018 •

edited

Loading