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

Bind arguments are now shown in a query #6

Merged
merged 6 commits into from Jun 16, 2017
Merged

Conversation

@dobrodushny
Copy link
Contributor

@dobrodushny dobrodushny commented Jun 9, 2017

Please, let me now, if it is needed to add more specs. Thanks!

Vladimir Katyurin and others added 4 commits Jun 7, 2017
Vladimir Katyurin
Specs for edge cases were implemented
@nepalez
Copy link
Owner

@nepalez nepalez commented Jun 13, 2017

Hi! Yes, I think specs should check what error messages are provided for various cases.

I'd also like to point out to this comment. Today I'm not sure that inserting bindings into query was a good decision, its better to display them separately like Rails does.

@dobrodushny
Copy link
Contributor Author

@dobrodushny dobrodushny commented Jun 13, 2017

Hi! I've seen that comment and the output format is like you've described there. If I'm wrong, plz correct me.

@nepalez
Copy link
Owner

@nepalez nepalez commented Jun 14, 2017

Ahh, yes, you're right.

I'm ok with a code. Still I think you should test that bindings are displayed. It's fine that you've created additional specs, but they cover main functionality (that matcher works per se ;) ), but not error messages

Vladimir Katyurin and others added 2 commits Jun 14, 2017
Specs for reporter were added
@dobrodushny
Copy link
Contributor Author

@dobrodushny dobrodushny commented Jun 14, 2017

Yeah, agreed with you :) Some specs are added for reporter class

@dobrodushny
Copy link
Contributor Author

@dobrodushny dobrodushny commented Jun 16, 2017

@nepalez could you please give some updates on pr status? Maybe something is required or it's ok in it's current state. Thanks!

@nepalez
Copy link
Owner

@nepalez nepalez commented Jun 16, 2017

Hi! Ahh, sorry, I've missed the change ))

@nepalez nepalez merged commit 556bb19 into nepalez:master Jun 16, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nepalez
Copy link
Owner

@nepalez nepalez commented Jun 16, 2017

@dobrodushny Thank you!
I'll make a release this night

@dobrodushny
Copy link
Contributor Author

@dobrodushny dobrodushny commented Jun 17, 2017

@nepalez Cool, thanks) It would be great if you add me as a helper on http://cultofmartians.com/ too :) Also, am I right that readme is need to be updated now?

@nepalez
Copy link
Owner

@nepalez nepalez commented Jun 17, 2017

Done
And back-reference for better craziness http://cultofmartians.com/tasks/adding-binds-to-rspec-sqlimit.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants