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

Added type hints to the Column class #3511

Merged
merged 1 commit into from
Apr 17, 2019

Conversation

morozov
Copy link
Member

@morozov morozov commented Apr 7, 2019

Q A
Type improvement
BC Break yes

lib/Doctrine/DBAL/Schema/Column.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Schema/Column.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Schema/Column.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Schema/Column.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Schema/Column.php Show resolved Hide resolved
lib/Doctrine/DBAL/Schema/Column.php Show resolved Hide resolved
lib/Doctrine/DBAL/Schema/Column.php Show resolved Hide resolved
@jwage
Copy link
Member

jwage commented Apr 17, 2019

Can we (should we?) separate the BC breaks due to adding type hints and BC breaks from us changing default behavior? IMO, while changing the defaults may be a good idea long term, I am not sure if making the change along side adding type hints is good. May be better to have 1 PR to add the type hints while maintaining behavior and then another PR to change the defaults. This way the defaults changing can be discussed in isolation from all the other changes and the type hints can probably be merged faster. If I am missing something, feel free to correct me. Just starting to get up to speed with the great work going on here.

@jwage jwage added this to the 3.0.0 milestone Apr 17, 2019
@morozov
Copy link
Member Author

morozov commented Apr 17, 2019

@jwage I get the intent but if we want to take this route, the changes have to be implemented in the reverse order:

  1. Drop the defaults (the real BC break)
  2. Enforce strict argument types (a break only for API implementors, not consumers).

The existing defaults only make sense with non-enforced argument types (hence the is_numeric() checks). If we introduce the strict types first, it will implicitly break the behavior covered by is_numeric().

I like the idea of the split in general. Especially, given that there are still some leftovers of the default value of 10 in Comparator. I'll open a separate PR.

@jwage jwage dismissed Ocramius’s stale review April 17, 2019 22:38

The feedback from Marco was addressed.

@morozov morozov merged commit 4ed75b4 into doctrine:develop Apr 17, 2019
@morozov morozov self-assigned this Apr 17, 2019
@morozov morozov deleted the column-type-hints branch April 17, 2019 22:42
morozov added a commit that referenced this pull request May 6, 2019
Added type hints to the Column class
morozov added a commit that referenced this pull request May 23, 2019
Added type hints to the Column class
morozov added a commit to morozov/dbal that referenced this pull request May 31, 2019
morozov added a commit to morozov/dbal that referenced this pull request May 31, 2019
morozov added a commit that referenced this pull request Jun 13, 2019
Added type hints to the Column class
morozov added a commit that referenced this pull request Jun 27, 2019
Added type hints to the Column class
morozov added a commit that referenced this pull request Jun 27, 2019
Added type hints to the Column class
morozov added a commit that referenced this pull request Jun 27, 2019
Added type hints to the Column class
morozov added a commit to morozov/dbal that referenced this pull request Aug 26, 2019
morozov added a commit to morozov/dbal that referenced this pull request Aug 27, 2019
morozov added a commit that referenced this pull request Nov 2, 2019
Added type hints to the Column class
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants