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

Upgraded Expression proposal #106

Merged
merged 12 commits into from
Mar 13, 2019
Merged

Upgraded Expression proposal #106

merged 12 commits into from
Mar 13, 2019

Conversation

dorantor
Copy link
Contributor

I did as we discussed here #101

Also added an example how it could be with functions, so functions would be properly wrapped and less reasons to use Expression\Raw. But naming convention in Clickhouse is not same as in PSR. So Class(and file) names should differ, I think. But call Expression\Function\UuidStringToNum($uuid) is not that clear, IMO. Another option is to alter it to Expression\Function('UUIDStringToNum', $uuid). It should cover all such cases, but will be less strict and, probably, less clear.

By the way, found that I'm not a contributor here https://github.com/smi2/phpClickHouse/graphs/contributors
after #101 was merged.

Is it by design or just a merging error? Besides that I needed code from #101 into master, so I could stop using fork in my projects and switch to the main repo, having visible contributions to other projects is a nice reward and really helps in career building.

@dorantor
Copy link
Contributor Author

BTW, Scrutinizer is complaining that

Variable "%s" not allowed in double quoted string; use sprintf() or concatenation instead

but it's the most readable way I can think of. Compare currently used

        return "UUIDStringToNum('{$this->uuid}')";

with

        return \sprintf('UUIDStringToNum(\'%s\')', $this->uuid);

or with

        return 'UUIDStringToNum(\'' . $this->uuid . '\')';

@isublimity isublimity merged commit a58ab5b into smi2:master Mar 13, 2019
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