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

support condition objects in utility functions #6666 #6685

Merged
merged 1 commit into from Nov 18, 2016

Conversation

5 participants
@jharting
Contributor

jharting commented Oct 12, 2016

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
    • I did not with mssql (don't have it available)
    • linting seems broken in master (13k problems)
  • Does your issue contain a link to existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
    • not yet, want to hear team's opinion on the change first
  • Have you added an entry under Future in the changelog?

NOTE: these things are not required to open a PR and can be done afterwards / while the PR is open.

Description of change

#6666

Makes it possible to pass condition objects (e.g. {foo: 'bar'}) to Sequelize.fn(), Sequelize.cast().
This makes it easy to use conditional counts in Sequelize queries, e.g.

Sequelize.fn('sum', {name: 'Fred'}) which results in SUM(``name``= 'Fred') which can be used in attribute definition to count entities for which the condition matches.

I did not get a chance to test with mssql as I do not have it available. If anyone can do that for me I would appreciate that.

var run = generator.handleSequelizeMethod.bind(generator);
var expectsql = Support.expectsql
if (Support.getTestDialect() !== 'mssql') {

This comment has been minimized.

@janmeier

janmeier Oct 17, 2016

Member

No need to skip mssql - It doesn't need to be 100 percent correct syntax (in my mind), as long as it tests the functionality (that you can pass nested conditions)

bar: 'bar'
}
}, 'int'))), {
default: 'SUM(CAST((`foo` = \'foo\' OR `bar` = \'bar\') AS INT))',

This comment has been minimized.

@janmeier

janmeier Oct 17, 2016

Member

Default should use [ and ] for escaping

@@ -226,4 +228,80 @@ describe(Support.getTestDialectTeaser('Utils'), function() {
expect(Utils.singularize('status')).to.equal('status');
});
});
describe('Sequelize.fn', function() {

This comment has been minimized.

@janmeier

janmeier Oct 17, 2016

Member

While an integration test might not be strictly needed, I think this is good to keep as an example of how the functionality can be used

@felixfbecker

This comment has been minimized.

Contributor

felixfbecker commented Oct 17, 2016

You can test MSSQL through AppVeyor ;)
Semicolon missing

@jharting jharting force-pushed the jharting:utils-object-param branch from b8bdf1a to a5a4988 Oct 25, 2016

@codecov-io

This comment has been minimized.

codecov-io commented Oct 25, 2016

Current coverage is 81.54% (diff: 100%)

Merging #6685 into master will decrease coverage by 13.08%

Powered by Codecov. Last update b32ce3c...f33c5ad

@jharting jharting force-pushed the jharting:utils-object-param branch from a5a4988 to 1c4200f Oct 25, 2016

@jharting

This comment has been minimized.

Contributor

jharting commented Oct 25, 2016

@janmeier @felixfbecker Thanks for feedback! I addressed some of the comments. I still do not have access to mssql (not even sure if it supports the syntax I am using in the tests). Let me try if I can use appveyor CI runner here to get the tests pass easily but maybe not.

@jharting jharting force-pushed the jharting:utils-object-param branch 3 times, most recently from f18691b to 4b89420 Oct 25, 2016

@jharting

This comment has been minimized.

Contributor

jharting commented Oct 25, 2016

I played with mssql and it does not seem to support this syntax. The closest alternative is SUM(CASE WHEN foo THEN 1 ELSE 0). Therefore, I put mssql exclusions into the integration tests.

@jharting jharting force-pushed the jharting:utils-object-param branch from 4b89420 to 56c70e8 Oct 25, 2016

@felixfbecker

This comment has been minimized.

Contributor

felixfbecker commented Oct 25, 2016

Don't forget to mention in the documentation then that this is not supported by MSSQL

@jharting jharting force-pushed the jharting:utils-object-param branch 2 times, most recently from 433ce5a to f33c5ad Oct 25, 2016

Support condition objects in utility functions #6666
#6666

Makes it possible to pass condition objects (e.g. {foo: 'bar'}) to Sequelize.fn(), Sequelize.cast().
This makes it easy to use conditional counts in Sequelize queries, e.g.

Sequelize.fn('sum', {name: 'Fred'}) which results in SUM(`name` = 'Fred') which can be used in attribute definition
to count entities for which the condition matches.

@jharting jharting force-pushed the jharting:utils-object-param branch from f33c5ad to 3329fad Nov 18, 2016

@jharting

This comment has been minimized.

Contributor

jharting commented Nov 18, 2016

I modified changelog and added mention to docs. From my POV this change is now finished. Let me know if you find anything missing.

@janmeier janmeier merged commit fc8fdef into sequelize:master Nov 18, 2016

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@janmeier

This comment has been minimized.

Member

janmeier commented Nov 18, 2016

Thanks 👍

@jharting

This comment has been minimized.

Contributor

jharting commented Nov 18, 2016

@janmeier Thank you! Btw is there a chance to get this into v3 as well? Should I open a new MR against v3? Do you plan further 3.x releases?

@janmeier

This comment has been minimized.

Member

janmeier commented Nov 18, 2016

Definitely - Let me see if I can cherry-pick it really quick

janmeier added a commit that referenced this pull request Nov 18, 2016

Support condition objects in utility functions #6666 (#6685)
#6666

Makes it possible to pass condition objects (e.g. {foo: 'bar'}) to Sequelize.fn(), Sequelize.cast().
This makes it easy to use conditional counts in Sequelize queries, e.g.

Sequelize.fn('sum', {name: 'Fred'}) which results in SUM(`name` = 'Fred') which can be used in attribute definition
to count entities for which the condition matches.
@janmeier

This comment has been minimized.

Member

janmeier commented Nov 18, 2016

75bd525, just going to wait for travis to do his thing before i push a new version

@janmeier

This comment has been minimized.

Member

janmeier commented Nov 18, 2016

v3.27.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment