Doctrine 2 - ORM
  1. Doctrine 2 - ORM
  2. DDC-274

Class and namespace naming inconsistency

    Details

    • Type: Improvement Improvement
    • Status: In Progress
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: 3.0
    • Component/s: None
    • Security Level: All
    • Labels:
      None

      Description

      There are inconsistencies with some class and namespace names that include acronyms.

      Examples:

      Classes with upper-casing:
      ORMException, DBALException, OCI8Connection, etc.

      Classes with proper-casing:
      RunDqlTask, CliException, MySqlPlatform, etc.

      Namespaces with upper-casing:
      DBAL, ORM, Doctrine\DBAL\Driver\PDOMsSql, etc.

      Namespaces with proper-casing:
      Doctrine\Common\Cli, Doctrine\DBAL\Tools\Cli\, Doctrine\ORM\Id, etc.

      There is more proper-casing than upper-casing. IMHO, proper-casing is better as it's easier to read "SqlException" than it is to read "SQLException" (the "E" looks like part of the acronym), and things like "CLITask" can be avoided.

      I discussed this a bit with Benjamin and Guilherme, and they were unsure and said that the whole team needed to reach consensus.

      I'm leaving the priority as "Major" because this should probably be fixed sooner rather than later to prevent compatibility breaks.

        Activity

        Glen Ainscow created issue -
        Hide
        Guilherme Blanco added a comment -

        Increasing the severity and adding a fix version since this MUST be fixed before next release.

        Show
        Guilherme Blanco added a comment - Increasing the severity and adding a fix version since this MUST be fixed before next release.
        Guilherme Blanco made changes -
        Field Original Value New Value
        Fix Version/s 2.0-BETA1 [ 10030 ]
        Priority Major [ 3 ] Critical [ 2 ]
        Hide
        Roman S. Borschel added a comment -

        I find this to be of rather minor importance. You're talking of compatibility breaks, but thats only the case if we intend to start being very nitpicky about the naming in the future. We are currently not, and we wont be, so not much risk of later breakage.

        We have a rule of thumb already: Acronyms up to 4 characters all uppercase, otherwise normal camelcase.

        Now, it is often a matter of taste what is an acronym and what not and also not all acronyms have a clear casing, prime example mysql.

        Being too nit-picky here is just a pain in the ass and we won't reach a common consensus anyway.

        Show
        Roman S. Borschel added a comment - I find this to be of rather minor importance. You're talking of compatibility breaks, but thats only the case if we intend to start being very nitpicky about the naming in the future. We are currently not, and we wont be, so not much risk of later breakage. We have a rule of thumb already: Acronyms up to 4 characters all uppercase, otherwise normal camelcase. Now, it is often a matter of taste what is an acronym and what not and also not all acronyms have a clear casing, prime example mysql. Being too nit-picky here is just a pain in the ass and we won't reach a common consensus anyway.
        Hide
        Roman S. Borschel added a comment -

        Oh and we better dont start arguing about whats better to read because there is no chance of agreement. If you ask me I find SQLException better than SqlException. So here we go. Lets better not run down this path.

        Show
        Roman S. Borschel added a comment - Oh and we better dont start arguing about whats better to read because there is no chance of agreement. If you ask me I find SQLException better than SqlException. So here we go. Lets better not run down this path.
        Hide
        Glen Ainscow added a comment -

        No need to get upset, I'm only trying to help.

        I just thought that consistency is usually a good idea, either way.

        I have to disagree in that an acronym is an acronym, it's not a matter of taste, the letters either stand for something, or they don't.

        As for "mysql", only the SQL part is an acronym. So, MySQL or MySql.

        Close if you disagree.

        Show
        Glen Ainscow added a comment - No need to get upset, I'm only trying to help. I just thought that consistency is usually a good idea, either way. I have to disagree in that an acronym is an acronym, it's not a matter of taste, the letters either stand for something, or they don't. As for "mysql", only the SQL part is an acronym. So, MySQL or MySql. Close if you disagree.
        Hide
        Roman S. Borschel added a comment - - edited

        I'm not upset. What made you think so? Maybe I should introduce a every now and then.

        There's just no point in arguing about readability.

        Like I said, we do have a naming standard even if its not adhered everywhere. The standard is also not something we've made up ourselves because we try to avoid that. When we introduced namespaces, we talked about adopting either the Java or .NET naming standards. We opted for the .NET standards. And there its recommended to write acronyms with up to 4 characters all uppercase.

        I dont think there are too many violations of that in the code, probably Cli => CLI and some others but most of it looks ok to me.

        UPDATE: Maybe we can gather a list here in this ticket of violations? Cli => CLI would be one. MySql => MySQL another. "Id" is rather an abbreviation of Identifier and not an acronym, to me at least.

        Show
        Roman S. Borschel added a comment - - edited I'm not upset. What made you think so? Maybe I should introduce a every now and then. There's just no point in arguing about readability. Like I said, we do have a naming standard even if its not adhered everywhere. The standard is also not something we've made up ourselves because we try to avoid that. When we introduced namespaces, we talked about adopting either the Java or .NET naming standards. We opted for the .NET standards. And there its recommended to write acronyms with up to 4 characters all uppercase. I dont think there are too many violations of that in the code, probably Cli => CLI and some others but most of it looks ok to me. UPDATE: Maybe we can gather a list here in this ticket of violations? Cli => CLI would be one. MySql => MySQL another. "Id" is rather an abbreviation of Identifier and not an acronym, to me at least.
        Hide
        Guilherme Blanco added a comment -

        It's not a case of getting anyone upset...

        But we should fix it and keep consistency of our codebase.

        @romanb We should all talk and reach a common sense.
        That's our last chance (last alpha) to get this consistency in.

        Show
        Guilherme Blanco added a comment - It's not a case of getting anyone upset... But we should fix it and keep consistency of our codebase. @romanb We should all talk and reach a common sense. That's our last chance (last alpha) to get this consistency in.
        Hide
        Roman S. Borschel added a comment -

        @Guilherme: We do have a naming standard since a year. You want to change the standard now?

        Show
        Roman S. Borschel added a comment - @Guilherme: We do have a naming standard since a year. You want to change the standard now?
        Hide
        Glen Ainscow added a comment -

        @Roman,

        Just a feeling I got.

        This issue was more about consistency (indicated in the title) than moving to proper-case naming.

        I think it might be up to 3 characters in .NET, HtmlElement, System.Linq, etc. Anyway, not important.

        I agree that Id. is an abbreviation.

        There are some more violations. If you decide you want to change them, let me know and I'll compile a list.

        Show
        Glen Ainscow added a comment - @Roman, Just a feeling I got. This issue was more about consistency (indicated in the title) than moving to proper-case naming. I think it might be up to 3 characters in .NET, HtmlElement, System.Linq, etc. Anyway, not important. I agree that Id. is an abbreviation. There are some more violations. If you decide you want to change them, let me know and I'll compile a list.
        Hide
        Roman S. Borschel added a comment -

        @Glen: Yes, a list would be great. I find it hard to be 100% consistent sometimes though because my taste conflicts with the rule. For example, I would prefer "Id" over "ID", especially since it comes directly after ORM "Doctrine\ORM\ID\..." would be a bit too much. But I would not like "Orm" or "Dbal" either. But I think in most cases we can clearly fix the inconsistency. Like CLI or MySQL or MSSQL (or should it be MsSQL?).

        Thanks for your help!

        We should probably create updated coding standards for Doctrine 2 also, since they differ quite some from Doctrine 1 due to the introduction of namespaces and such. I'll create a ticket for that.

        Show
        Roman S. Borschel added a comment - @Glen: Yes, a list would be great. I find it hard to be 100% consistent sometimes though because my taste conflicts with the rule. For example, I would prefer "Id" over "ID", especially since it comes directly after ORM "Doctrine\ORM\ID\..." would be a bit too much. But I would not like "Orm" or "Dbal" either. But I think in most cases we can clearly fix the inconsistency. Like CLI or MySQL or MSSQL (or should it be MsSQL?). Thanks for your help! We should probably create updated coding standards for Doctrine 2 also, since they differ quite some from Doctrine 1 due to the introduction of namespaces and such. I'll create a ticket for that.
        Hide
        Glen Ainscow added a comment -

        I'll do the list for you by tomorrow at the latest, just running out of time ATM.

        Id is correct, as mentioned above, so that would be fine. MsSQL is correct (Ms = Microsoft = abbreviation).

        Show
        Glen Ainscow added a comment - I'll do the list for you by tomorrow at the latest, just running out of time ATM. Id is correct, as mentioned above, so that would be fine. MsSQL is correct (Ms = Microsoft = abbreviation).
        Hide
        Glen Ainscow added a comment -

        Found some time ...

        Doctrine\Common\Cache\ApcCache -> APCCache
        Doctrine\Common\Cli -> CLI
        Doctrine\Common\Cli\Printers\AnsiColorPrinter -> ANSIColorPrinter
        Doctrine\Common\Cli\CliController -> CLIController
        Doctrine\Common\Cli\CliException -> CLIException

        Doctrine\DBAL\Driver\PDOMsSql -> PDOMsSQL
        Doctrine\DBAL\Driver\PDOMySql -> PDOMySQL
        Doctrine\DBAL\Driver\PDOPgSql -> PDOPgSQL
        Doctrine\DBAL\Driver\PDOSqlite -> PDOSQLite
        Doctrine\DBAL\Logging\EchoSqlLogger -> EchoSQLLogger
        Doctrine\DBAL\Logging\SqlLogger -> SQLLogger
        Doctrine\DBAL\Platforms\MsSqlPlatform -> MsSQLPlatform
        Doctrine\DBAL\Platforms\MySqlPlatform -> MySQLPlatform
        Doctrine\DBAL\Platforms\PostgreSqlPlatform -> PostgreSQLPlatform
        Doctrine\DBAL\Platforms\SqlitePlatform -> SQLitePlatform
        Doctrine\DBAL\Schema\Visitor\CreateSchemaSqlCollector -> CreateSchemaSQLCollector
        Doctrine\DBAL\Schema\Visitor\DropSchemaSqlCollector -> DropSchemaSQLCollector
        Doctrine\DBAL\Schema\MsSqlSchemaManager -> MsSQLSchemaManager
        Doctrine\DBAL\Schema\MySqlSchemaManager -> MySQLSchemaManager
        Doctrine\DBAL\Schema\PostgreSqlSchemaManager -> PostgreSQLSchemaManager
        Doctrine\DBAL\Schema\SqliteSchemaManager -> SQLiteSchemaManager
        Doctrine\DBAL\Tools\Cli -> CLI
        Doctrine\DBAL\Tools\Cli\Tasks\RunSqlTask -> RunSQLTask

        Doctrine\ORM\Mapping\Driver\PhpDriver -> PHPDriver
        Doctrine\ORM\Mapping\Driver\XmlDriver -> XMLDriver
        Doctrine\ORM\Mapping\Driver\YamlDriver -> YAMLDriver
        Doctrine\ORM\Query\Exec\AbstractSqlExecutor -> AbstractSQLExecutor
        (Should Doctrine\ORM\Query\Expr\Andx and Orx be AndX and OrX?)
        Doctrine\ORM\Query\SqlWalker -> SQLWalker
        Doctrine\ORM\Tools\Cli -> CLI
        Doctrine\ORM\Tools\Cli\Tasks\RunDqlTask -> RunDQLTask
        Doctrine\ORM\Tools\Export\Driver\PhpExporter -> PHPExporter
        Doctrine\ORM\Tools\Export\Driver\XmlExporter -> XMLExporter
        Doctrine\ORM\Tools\Export\Driver\YamlExporter -> YAMLExporter

        ... I think that's it. More than you expected? I did say: "There is more proper-casing than upper-casing."

        Show
        Glen Ainscow added a comment - Found some time ... Doctrine\Common\Cache\ApcCache -> APCCache Doctrine\Common\Cli -> CLI Doctrine\Common\Cli\Printers\AnsiColorPrinter -> ANSIColorPrinter Doctrine\Common\Cli\CliController -> CLIController Doctrine\Common\Cli\CliException -> CLIException Doctrine\DBAL\Driver\PDOMsSql -> PDOMsSQL Doctrine\DBAL\Driver\PDOMySql -> PDOMySQL Doctrine\DBAL\Driver\PDOPgSql -> PDOPgSQL Doctrine\DBAL\Driver\PDOSqlite -> PDOSQLite Doctrine\DBAL\Logging\EchoSqlLogger -> EchoSQLLogger Doctrine\DBAL\Logging\SqlLogger -> SQLLogger Doctrine\DBAL\Platforms\MsSqlPlatform -> MsSQLPlatform Doctrine\DBAL\Platforms\MySqlPlatform -> MySQLPlatform Doctrine\DBAL\Platforms\PostgreSqlPlatform -> PostgreSQLPlatform Doctrine\DBAL\Platforms\SqlitePlatform -> SQLitePlatform Doctrine\DBAL\Schema\Visitor\CreateSchemaSqlCollector -> CreateSchemaSQLCollector Doctrine\DBAL\Schema\Visitor\DropSchemaSqlCollector -> DropSchemaSQLCollector Doctrine\DBAL\Schema\MsSqlSchemaManager -> MsSQLSchemaManager Doctrine\DBAL\Schema\MySqlSchemaManager -> MySQLSchemaManager Doctrine\DBAL\Schema\PostgreSqlSchemaManager -> PostgreSQLSchemaManager Doctrine\DBAL\Schema\SqliteSchemaManager -> SQLiteSchemaManager Doctrine\DBAL\Tools\Cli -> CLI Doctrine\DBAL\Tools\Cli\Tasks\RunSqlTask -> RunSQLTask Doctrine\ORM\Mapping\Driver\PhpDriver -> PHPDriver Doctrine\ORM\Mapping\Driver\XmlDriver -> XMLDriver Doctrine\ORM\Mapping\Driver\YamlDriver -> YAMLDriver Doctrine\ORM\Query\Exec\AbstractSqlExecutor -> AbstractSQLExecutor (Should Doctrine\ORM\Query\Expr\Andx and Orx be AndX and OrX?) Doctrine\ORM\Query\SqlWalker -> SQLWalker Doctrine\ORM\Tools\Cli -> CLI Doctrine\ORM\Tools\Cli\Tasks\RunDqlTask -> RunDQLTask Doctrine\ORM\Tools\Export\Driver\PhpExporter -> PHPExporter Doctrine\ORM\Tools\Export\Driver\XmlExporter -> XMLExporter Doctrine\ORM\Tools\Export\Driver\YamlExporter -> YAMLExporter ... I think that's it. More than you expected? I did say: "There is more proper-casing than upper-casing."
        Hide
        Roman S. Borschel added a comment -

        No, not more than I expected. It's mostly SQL and CLI basically. Thanks for the list!

        We should try to go through that list before beta.

        Show
        Roman S. Borschel added a comment - No, not more than I expected. It's mostly SQL and CLI basically. Thanks for the list! We should try to go through that list before beta.
        Roman S. Borschel made changes -
        Status Open [ 1 ] In Progress [ 3 ]
        Hide
        Roman S. Borschel added a comment -

        Methods should be adjusted accordingly also (even though they're case insensitive). I already started with that. More to come.

        Show
        Roman S. Borschel added a comment - Methods should be adjusted accordingly also (even though they're case insensitive). I already started with that. More to come.
        Hide
        Roman S. Borschel added a comment -

        Most of the Cli => CLI changes seem to be complete now.

        Show
        Roman S. Borschel added a comment - Most of the Cli => CLI changes seem to be complete now.
        Hide
        Roman S. Borschel added a comment -

        Pushing the rest of the name changes towards beta2.

        Show
        Roman S. Borschel added a comment - Pushing the rest of the name changes towards beta2.
        Roman S. Borschel made changes -
        Fix Version/s 2.0-BETA2 [ 10050 ]
        Fix Version/s 2.0-BETA1 [ 10030 ]
        Hide
        Roman S. Borschel added a comment -

        More renamings have been done but still more to do. Pushing remaining work to beta3.

        Show
        Roman S. Borschel added a comment - More renamings have been done but still more to do. Pushing remaining work to beta3.
        Roman S. Borschel made changes -
        Fix Version/s 2.0-BETA3 [ 10060 ]
        Fix Version/s 2.0-BETA2 [ 10050 ]
        Roman S. Borschel made changes -
        Fix Version/s 2.0-BETA4 [ 10072 ]
        Fix Version/s 2.0-BETA3 [ 10060 ]
        Hide
        Roman S. Borschel added a comment -

        Final name changes should be done prior to going into RC1.

        Show
        Roman S. Borschel added a comment - Final name changes should be done prior to going into RC1.
        Roman S. Borschel made changes -
        Fix Version/s 2.0-RC1 [ 10091 ]
        Fix Version/s 2.0-BETA4 [ 10072 ]
        Hide
        Benjamin Eberlei added a comment -

        there is much to be done yet, however most of it is also public API class names and might cause quite some BC breaks (i.e. DBAL Platforms and Schema Manager, Mapping Driver Names). I am not sure how to proceed here.

        Show
        Benjamin Eberlei added a comment - there is much to be done yet, however most of it is also public API class names and might cause quite some BC breaks (i.e. DBAL Platforms and Schema Manager, Mapping Driver Names). I am not sure how to proceed here.
        Hide
        Roman S. Borschel added a comment -

        Since PHP is mostly case-insensitive on class and method names it shouldn't be much of an issue.

        Show
        Roman S. Borschel added a comment - Since PHP is mostly case-insensitive on class and method names it shouldn't be much of an issue.
        Benjamin Eberlei made changes -
        Workflow jira [ 10769 ] jira-feedback [ 14095 ]
        Benjamin Eberlei made changes -
        Workflow jira-feedback [ 14095 ] jira-feedback2 [ 15959 ]
        Benjamin Eberlei made changes -
        Workflow jira-feedback2 [ 15959 ] jira-feedback3 [ 18213 ]
        Guilherme Blanco made changes -
        Assignee Roman S. Borschel [ romanb ] Guilherme Blanco [ guilhermeblanco ]
        Fix Version/s 3.0 [ 10129 ]
        Fix Version/s 2.0-RC1 [ 10091 ]
        Affects Version/s 2.0-ALPHA4 [ 10036 ]
        Hide
        Guilherme Blanco added a comment -

        Scheduled to 3.0 as this may potentially create BC breaks

        Show
        Guilherme Blanco added a comment - Scheduled to 3.0 as this may potentially create BC breaks
        Marco Pivetta made changes -
        Priority Critical [ 2 ] Major [ 3 ]

        This list may be incomplete, as errors occurred whilst retrieving source from linked applications:

        • Request to http://www.doctrine-project.org/fisheye/ failed: Error in remote call to 'FishEye 0 (http://www.doctrine-project.org/fisheye/)' (http://www.doctrine-project.org/fisheye) [AbstractRestCommand{path='/rest-service-fe/search-v1/crossRepositoryQuery', params={query=DDC-274, expand=changesets[0:20].revisions[0:29],reviews}, methodType=GET}] : Received status code 503 (Service Temporarily Unavailable)

          People

          • Assignee:
            Guilherme Blanco
            Reporter:
            Glen Ainscow
          • Votes:
            1 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated: