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

Bugfix/query event sql string #2566

Merged
merged 2 commits into from Apr 12, 2018

Conversation

Projects
None yet
2 participants
@wubzz
Collaborator

wubzz commented Apr 11, 2018

Fixes #2549

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Apr 11, 2018

Looks good. Should this be marked as breaking change?

Now that there is test for the format I think that should be mentioned in documentation. I just looked those event docs and they seem pretty bad, not telling at all what kind of data is passed to event handler.

@wubzz

This comment has been minimized.

Collaborator

wubzz commented Apr 12, 2018

I don't believe this is a breaking change, it should be quite the opposite since it restores previous functionality.

I created knex/documentation#103 for docs as a seperate issue. As mentioned in #2549 it needs a complete overhaul, really.

This being the only thing merged since 0.14.5 I'll try to publish 0.14.6 later if I can.

@wubzz wubzz closed this Apr 12, 2018

@wubzz wubzz reopened this Apr 12, 2018

@wubzz wubzz merged commit 8488e22 into tgriesser:master Apr 12, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@elhigu

This comment has been minimized.

Collaborator

elhigu commented Apr 12, 2018

I don't believe this is a breaking change, it should be quite the opposite since it restores previous functionality.

Yep, this is one of those less clear cases, when functionality is changed, but since it hadn't been documented earlier, its kind of 50%/50% if it should be marked as breaking or not... especially because last time it was changed by accident.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Apr 18, 2018

I think I'll make next knex release to be 0.15 and add this to one of breaking changes and explain in log that this has been gone back and forward since 0.13.0 (this has clearly caused issue to someone, so I think reporting this change wont hurt). Maybe there will be some real breaking changes too.

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