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

Made it possible for the TO_* functions to take more varied arguments #48

Merged
merged 6 commits into from
May 29, 2019

Conversation

magnusnordlander
Copy link
Contributor

We want to be able to pass any kind of StringPrimary into the TO_JSONB function, however, I think it makes sense to be able to pass e.g. a SimpleArithmeticExpression as well, and it seems NewValue is the best corresponding type for that.

Also, even though we're only using it in TO_JSONB, obviously this makes sense for the other TO_* functions too.

@coveralls
Copy link

coveralls commented May 28, 2019

Pull Request Test Coverage Report for Build 234

  • 2 of 2 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+2.4%) to 94.479%

Totals Coverage Status
Change from base Build 227: 2.4%
Covered Lines: 308
Relevant Lines: 326

💛 - Coveralls

@martin-georgiev
Copy link
Owner

martin-georgiev commented May 28, 2019

Would you please add new tests for this?

@magnusnordlander
Copy link
Contributor Author

Sure, I have now added test cases for some of the other parser types that NewValue allows.

'SELECT to_tsvector(LOWER(c0_.text)) AS sclr_0 FROM ContainsText c0_',
'SELECT to_tsvector(1 + 1) AS sclr_0 FROM ContainsText c0_',
'SELECT to_tsvector(1) AS sclr_0 FROM ContainsText c0_',
'SELECT to_tsvector(LENGTH(c0_.text)) AS sclr_0 FROM ContainsText c0_',
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The checks from lines 24, 25 and 26 won't produce meaningful SQL, as to_tsvector(integer) is not a valid function. You can safely drop those scenarios.

\sprintf('SELECT TO_TSQUERY(UPPER(e.text)) FROM %s e', ContainsText::class),
\sprintf('SELECT TO_TSQUERY(1+1) FROM %s e', ContainsText::class),
\sprintf('SELECT TO_TSQUERY(true) FROM %s e', ContainsText::class),
\sprintf('SELECT TO_TSQUERY(LENGTH(e.text)) FROM %s e', ContainsText::class),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The checks from lines 35, 36 and 37 won't produce meaningful SQL, as to_tsquery(integer) is not a valid function. You can safely drop those scenarios.

@magnusnordlander
Copy link
Contributor Author

I suppose that considering that to_tsquery and to_tsvector only takes strings, this doesn't make much sense for them, so i've undone those changes. I added the same tests for to_json and to_jsonb though, since the accept anyelement.

@martin-georgiev
Copy link
Owner

Thank you @magnusnordlander for this contribution! It's lovely to see new people enhancing the project.

@martin-georgiev martin-georgiev merged commit b8acbb0 into martin-georgiev:develop May 29, 2019
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.

3 participants