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

[WIP] Sql Server DDL #231

Open
wants to merge 40 commits into
base: develop
from

Conversation

Projects
None yet
4 participants
@alextech
Contributor

alextech commented Mar 11, 2017

#215, #214 as starting points plus everything else

I am following existing style of specification compilation to make code reviewer easier, even though it is extremely repetitive, eg. altertable decorator vs createtable decorator. I assume maintainers would have already gotten used to looking at current structure, plus current style is known to work and I do not want to add extra uncertainty of reorganizing inheritance hierarchy on top of trying to get syntax correct.

Current DDL implementation is extremely MySQL biased - feels like it is Zend MySql instead of Zend DB, it crashes on simplest of commands, so this project will take some time to complete and will be a large code review. Therefore, probably won't make it next release but still want to show that I am not slacking off too much.

@alextech

This comment has been minimized.

Show comment
Hide comment
@alextech

alextech Mar 11, 2017

Contributor

More complex options like encrypted with and masked with can be considered to be dropped. I put them in for completion sake of available options, but they have their own special syntax for values that might not look good without validation and will be completely up to user's responsibility to get right.

Contributor

alextech commented Mar 11, 2017

More complex options like encrypted with and masked with can be considered to be dropped. I put them in for completion sake of available options, but they have their own special syntax for values that might not look good without validation and will be completely up to user's responsibility to get right.

@alextech alextech changed the title from Sql Server DDL to [WIP] Sql Server DDL Mar 11, 2017

@alextech

This comment has been minimized.

Show comment
Hide comment
@alextech

alextech Mar 11, 2017

Contributor

@ezimuel what are SqlFunctionalTest.php? How are they different from tests of objects themselves? Where should I ideally put my assertions?

Thanks for the guidance!

Contributor

alextech commented Mar 11, 2017

@ezimuel what are SqlFunctionalTest.php? How are they different from tests of objects themselves? Where should I ideally put my assertions?

Thanks for the guidance!

@froschdesign

This comment has been minimized.

Show comment
Hide comment
@froschdesign

froschdesign Mar 11, 2017

Member

@alextech
I will not add comments to your code, because it will create to much noise and it is out of scope at this point. Here are some points to consider:

  • never use abbreviation for variable or property names (e.g. $ctd, $ct, $insertPos, $coName, $coValue, $pk, …)
  • add always a DocBlock to every property and method
  • add the correct license header to every file
  • use the alias TestCase for PHPUnit_Framework_TestCase and an update to newer PHPUnit version becomes easier
  • if you include links in DocBlocks please use @see
  • do not add a blank line between @param and @return
  • break long lines
  • pay attention to the brackets
Member

froschdesign commented Mar 11, 2017

@alextech
I will not add comments to your code, because it will create to much noise and it is out of scope at this point. Here are some points to consider:

  • never use abbreviation for variable or property names (e.g. $ctd, $ct, $insertPos, $coName, $coValue, $pk, …)
  • add always a DocBlock to every property and method
  • add the correct license header to every file
  • use the alias TestCase for PHPUnit_Framework_TestCase and an update to newer PHPUnit version becomes easier
  • if you include links in DocBlocks please use @see
  • do not add a blank line between @param and @return
  • break long lines
  • pay attention to the brackets
@alextech

This comment has been minimized.

Show comment
Hide comment
@alextech

alextech Mar 11, 2017

Contributor

@froschdesign Great reminders. Sorry, not polished enough. I did use CS fixer script against all files thinking it has the correct coding standard details, which seem to contradict to your review in this and other PR. Are they out of date?

Contributor

alextech commented Mar 11, 2017

@froschdesign Great reminders. Sorry, not polished enough. I did use CS fixer script against all files thinking it has the correct coding standard details, which seem to contradict to your review in this and other PR. Are they out of date?

@froschdesign

This comment has been minimized.

Show comment
Hide comment
@froschdesign

froschdesign Mar 11, 2017

Member

@alextech
The current ruleset can be found at zendframework/zend-coding-standard and a complete coding standard is work in progress.

Member

froschdesign commented Mar 11, 2017

@alextech
The current ruleset can be found at zendframework/zend-coding-standard and a complete coding standard is work in progress.

@alextech

This comment has been minimized.

Show comment
Hide comment
@alextech

alextech Mar 11, 2017

Contributor

@froschdesign Thanks for your patience. What to do if code is heavily based on past work? This is not strictly new code. Fixing styles of, say much needed better variable names, will involve going into codebase beyond the scope of PR to keep consistent. This is the worst case I came up on where syntax generation is more of a pattern, than reusable architecture. So if I take it too far from the original MySQL implementation in its style, inconsistencies are introduced which are even harder to deal with than duplication, trying to understand why and how every platform class. looks different. But if I update coding standard in original sources along with updates to keep consistent, then its questionable to have MySQL platform updates in PR for SqlServer, then PostgreSQL. Do you have a procedure for this?

Contributor

alextech commented Mar 11, 2017

@froschdesign Thanks for your patience. What to do if code is heavily based on past work? This is not strictly new code. Fixing styles of, say much needed better variable names, will involve going into codebase beyond the scope of PR to keep consistent. This is the worst case I came up on where syntax generation is more of a pattern, than reusable architecture. So if I take it too far from the original MySQL implementation in its style, inconsistencies are introduced which are even harder to deal with than duplication, trying to understand why and how every platform class. looks different. But if I update coding standard in original sources along with updates to keep consistent, then its questionable to have MySQL platform updates in PR for SqlServer, then PostgreSQL. Do you have a procedure for this?

@froschdesign

This comment has been minimized.

Show comment
Hide comment
@froschdesign

froschdesign Mar 11, 2017

Member

What to do if code is heavily based on past work? This is not strictly new code.

Update old code and their style / syntax in an another PR. This PR is titled "Sql Server DDL" and has nothing to do with "Update code or formatting style".

My points to the code style only belongs to your newly added classes and methods. For example: ZendTest\Db\Sql\Platform\SqlServer\Ddl\AlterTableDecoratorTest::testGetSqlString()
There you use these meaninglessly and ugly variable names!


Again: I will not generate to much unnecessary noise in your PR. Use the coding standard on newly added code and on any parts that you must rework. Ignore the rest.

Member

froschdesign commented Mar 11, 2017

What to do if code is heavily based on past work? This is not strictly new code.

Update old code and their style / syntax in an another PR. This PR is titled "Sql Server DDL" and has nothing to do with "Update code or formatting style".

My points to the code style only belongs to your newly added classes and methods. For example: ZendTest\Db\Sql\Platform\SqlServer\Ddl\AlterTableDecoratorTest::testGetSqlString()
There you use these meaninglessly and ugly variable names!


Again: I will not generate to much unnecessary noise in your PR. Use the coding standard on newly added code and on any parts that you must rework. Ignore the rest.

@alextech

This comment has been minimized.

Show comment
Hide comment
@alextech

alextech Apr 30, 2017

Contributor

@ezimuel is this planned for 2.x release or 3.x ? If later, should I see if conversion to 7.1 is feasible?
Also not sure what to do with lower percentage hit for some of the functions reported by coveralls. For example, processColumns() switch block does have all cases hit.

Contributor

alextech commented Apr 30, 2017

@ezimuel is this planned for 2.x release or 3.x ? If later, should I see if conversion to 7.1 is feasible?
Also not sure what to do with lower percentage hit for some of the functions reported by coveralls. For example, processColumns() switch block does have all cases hit.

@ezimuel

This comment has been minimized.

Show comment
Hide comment
@ezimuel

ezimuel Nov 23, 2017

Member

@alextech are you still interested in complete this PR? If so, I think we should schedule it for 3.0. Let me know if you need any assistance.

Member

ezimuel commented Nov 23, 2017

@alextech are you still interested in complete this PR? If so, I think we should schedule it for 3.0. Let me know if you need any assistance.

@alextech

This comment has been minimized.

Show comment
Hide comment
@alextech

alextech Nov 23, 2017

Contributor

@ezimuel I am. I was keeping WIP label while hoping to get wider testing usecases from wider slack community and mathew's weekly newsletter. I was looking for more API usage combinations in unit tests I may not have thought of on my own and missed, and more stress test against actual platform. So that is something you could potentially assist with if can.

Contributor

alextech commented Nov 23, 2017

@ezimuel I am. I was keeping WIP label while hoping to get wider testing usecases from wider slack community and mathew's weekly newsletter. I was looking for more API usage combinations in unit tests I may not have thought of on my own and missed, and more stress test against actual platform. So that is something you could potentially assist with if can.

@ezimuel ezimuel self-assigned this Dec 4, 2017

@ezimuel ezimuel added this to the 3.0.0 milestone Dec 4, 2017

@alextech

This comment has been minimized.

Show comment
Hide comment
@alextech

alextech Dec 8, 2017

Contributor

Sorry looks like I am horrible at rebasing. I thought it will re-push only commits that had conflicts :(

@ezimuel How do new features like this work for PHP 7.2, specifically return types? Can I use return types in my new features and let everything else gradually migrate over time or you rather have consistency and either whole codebase uses types, or none at all. If former, should I do a quick PR into development branch to take 5.6 from travis the way I see some other repositories have?

Contributor

alextech commented Dec 8, 2017

Sorry looks like I am horrible at rebasing. I thought it will re-push only commits that had conflicts :(

@ezimuel How do new features like this work for PHP 7.2, specifically return types? Can I use return types in my new features and let everything else gradually migrate over time or you rather have consistency and either whole codebase uses types, or none at all. If former, should I do a quick PR into development branch to take 5.6 from travis the way I see some other repositories have?

alextech and others added some commits Dec 8, 2017

sasha sasha
Merge remote-tracking branch 'origin/feature/sqlserver_ddl' into feat…
…ure/sqlserver_ddl

# Conflicts:
#	test/Sql/Platform/SqlServer/Ddl/AlterTableDecoratorTest.php
Merge remote-tracking branch 'origin/feature/sqlserver_ddl' into feat…
…ure/sqlserver_ddl

# Conflicts:
#	test/Sql/Platform/SqlServer/Ddl/AlterTableDecoratorTest.php
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment