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 case when syntax #19

Merged
merged 10 commits into from
Nov 30, 2023
Merged

Conversation

morloderex
Copy link
Contributor

Hello everybody.

This is a an implementation of the last reply from @tpetry in this discussion.

Implementation details

Unfortunately PHP itself has reserved the case as a keyword, meaning the word case is not allowed as a PHP classname.
Which is why i have chosen to use CaseBlock instead. However i am open for suggestions regarding naming for this class.

@morloderex morloderex changed the title Case when syntax Add case when syntax Oct 25, 2023
@tpetry
Copy link
Owner

tpetry commented Oct 25, 2023

Its looking good. Give me a few days to think about it. I am currently not sure whether we should call it case as most devs either don't know of that SQL constuct. If would be more easy to understand, but has the same issue about reserved keywords.

@morloderex
Copy link
Contributor Author

morloderex commented Oct 25, 2023

@tpetry, thank you for the feedback. I see some failing tests. It seems to just be the expected sql that's not correct in these test cases. I'll take a look.

Regarding the naming:
When you mentioned if, my first thought that came to my mind was IfBlock or SwitchBlock.

Block sense it is a block of if else statements so to keep the terminology in regards to PHP code blocks.

@morloderex
Copy link
Contributor Author

morloderex commented Oct 25, 2023

@tpetry I have fixed the failing test it was as i suspected just a minor sql expectation issue using "" instead of [] for the MsSql case for one test.

This pr should be good to go from a code perspective now it's only naming convention of the classes that is left to be discussed.

@tpetry
Copy link
Owner

tpetry commented Nov 1, 2023

I made some improvements:

  • CaseBlock::else is optional
  • CaseCondition::condition removed string type as a string value would be escaped to use as a column. That doesnt make any sense
  • CaseCondition::condition is now using the ConditionExpression to make clear that only conditions are allowed
  • I made the tests more generic by not relying on other expressions as failures with them would also report failures for the case

Are you ok with these? Did I not see any issues?


I am still not sure whether to name them CaseBlock and CaseCondition. They are obvious but still don't feel good. I am thinking of more like a general term like if, condition or something like that. But not quite sure. They can't be changed once released...

@morloderex
Copy link
Contributor Author

morloderex commented Nov 2, 2023

@tpetry Thank you for looking over this pr again.
I am totally fine with your improvements.


How do you feel about my proposal before about naming it SwitchBlock and Condition?

@morloderex
Copy link
Contributor Author

morloderex commented Nov 10, 2023

@tpetry Sorry to bother you about this. But any thoughts here about my last proposal?

@tpetry
Copy link
Owner

tpetry commented Nov 10, 2023

Its still on my mind. I want to ask the Twitter audience later today what they think.

It will definitely be merged. I am just unsure whether to change the names. I want to make the package 1.0.0 in the following weeks - so there won't be any more time to change it once merged.

@morloderex
Copy link
Contributor Author

@tpetry I have seen your twitter post and it seems like it didn't get enough feedback.

I would really love to get this in soon-ish, It's a real shame that Case is a reserved word for class names in PHP.

@tpetry tpetry force-pushed the feature/case-when-blocks branch from c4f8181 to 75c4424 Compare November 30, 2023 08:36
@tpetry tpetry merged commit 580a7a0 into tpetry:main Nov 30, 2023
121 of 122 checks passed
@tpetry
Copy link
Owner

tpetry commented Nov 30, 2023

Released as 0.9.0

@morloderex morloderex deleted the feature/case-when-blocks branch November 30, 2023 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants