Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat(database): add set data type #725

Merged
merged 5 commits into from
Nov 16, 2024
Merged

Conversation

MrYamous
Copy link
Contributor

Part of #547 to add the set data type to the table statements

To be honest I have a doubt about this implementation.
I think it can be confusing for end users to silently ignore this data type if not supported by the database. I did so inspired by BelongsTo or DropConstraint statements but I prefer the way it's done in ShowTables which throw an UnsupportedDialect exception.
But this broke tests (or at least, I don't know how to handle multiples cases from a Dialect to an other)

Btw I think it can be a good point to try to be consistent in how handle this situation

@brendt
Copy link
Member

brendt commented Nov 14, 2024

I think it can be confusing for end users to silently ignore this data type if not supported by the database.

I agree.

But this broke tests (or at least, I don't know how to handle multiples cases from a Dialect to an other)

I want to help figure this out with you, because indeed it's important to get this right. Could you maybe push the version with the exception, that way I can play around with it some more and see what happens in the tests

@brendt brendt marked this pull request as draft November 14, 2024 09:18
@MrYamous
Copy link
Contributor Author

I think it can be confusing for end users to silently ignore this data type if not supported by the database.

I agree.

But this broke tests (or at least, I don't know how to handle multiples cases from a Dialect to an other)

I want to help figure this out with you, because indeed it's important to get this right. Could you maybe push the version with the exception, that way I can play around with it some more and see what happens in the tests

@brendt I've come up with an idea to deal with this situation, so I'm back with a new version that tests all the cases, including UnsupportedDialect Exception, and green CI :) but that I find very verbose

@brendt
Copy link
Member

brendt commented Nov 15, 2024

This looks good, what did you find too verbose about it?

@MrYamous MrYamous marked this pull request as ready for review November 15, 2024 13:35
@MrYamous
Copy link
Contributor Author

This looks good, what did you find too verbose about it?

Nevermind, if it's good for you it's good for me

I'll send another PR a bit later to use this Exception in other statements for consistency as suggested in my first message

@brendt brendt merged commit f0db5c8 into tempestphp:main Nov 16, 2024
57 checks passed
@brendt
Copy link
Member

brendt commented Nov 16, 2024

Perfect, thanks!

@MrYamous MrYamous deleted the database/set branch November 16, 2024 07:57
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants