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

Handle default database for persistent connection #561

Merged
merged 1 commit into from
Mar 18, 2020
Merged

Handle default database for persistent connection #561

merged 1 commit into from
Mar 18, 2020

Conversation

abarkine
Copy link
Contributor

This change is a fix for change #462 . Specifying both default database (as 0) and persistent (as true) results in falsy persistent setting. In mentioned PR, there is a mentioned predis change, which works only if persistent key is not boolean.

Since we are interested in persistent => true cases, it is required to check if mentioned database is falsy or not.

@B-Galati
Copy link
Collaborator

B-Galati commented Mar 13, 2020

Thanks! LGTM.

Could you rebase your branch please and we will be good to go.

@abarkine
Copy link
Contributor Author

Done

@B-Galati
Copy link
Collaborator

Something is wrong the branch contains 11 commits

@abarkine
Copy link
Contributor Author

@B-Galati i run git remote add upstream https://github.com/snc/SncRedisBundle and git rebase upstream/master. Which step was wrong, how can I solve it? Should I create new PR?

@B-Galati
Copy link
Collaborator

No need for another MR.

Do git rebase -i upstream/master and remove lines that do not belong to your MR.

@abarkine
Copy link
Contributor Author

Should be good now hopefully.

@B-Galati
Copy link
Collaborator

No it's not, it should have one commit only.

did you git fetch before? Make sure to have the option -i on rebase.

@abarkine
Copy link
Contributor Author

Now?

@B-Galati
Copy link
Collaborator

Perfect, thanks @asilelik.

@B-Galati B-Galati merged commit a9444a5 into snc:master Mar 18, 2020
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