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

Add arguments for timestamps columns names. #707

Closed

Conversation

samizdam
Copy link
Contributor

@samizdam samizdam commented Dec 5, 2015

Implement #576 && #706

@samizdam
Copy link
Contributor Author

samizdam commented Dec 5, 2015

@robmorgan I don't sure, but this feature will be applied only after phar rebuilding?
Do I must push with pull request binary file?
Contributing FAQ not provide some info about it...

@shadowhand
Copy link
Contributor

@samizdam please add tests.

@samizdam
Copy link
Contributor Author

@shadowhand this method is already covered.
I'm update my branch with upstream now.

table-coverage

@robmorgan
Copy link
Member

I dont really think we need this feature. If people want to change types etc or column names I generally tell them just to use addColumn() instead. I'd rather leave this method simple like Rails.

@shadowhand
Copy link
Contributor

I don't see any problem with having defaults that can be modified rather than hard coded. Seems like a reasonable approach to me.

rquadling added a commit that referenced this pull request Jul 14, 2016
@rquadling
Copy link
Collaborator

I've implemented this feature, along with appropriate guarding of null (you cannot have a column called null - who'd have thought that!) and unit tests.

@rquadling rquadling closed this Jul 14, 2016
@samizdam samizdam deleted the i-706-timestamp-columns-names branch November 9, 2018 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants