Skip to content

Fixes #70 - Default options are overriding command line defined options #400

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

Closed
wants to merge 5 commits into from

Conversation

ogirardot
Copy link
Contributor

Test is included to check that the timeout default for the socket now is clearly defined.
Any comment is welcomed.

Regards,

Olivier.

@carljm
Copy link
Contributor

carljm commented Dec 12, 2011

Hi @ssaboum - thanks for these fixes.

Note that a pull request on github includes all commits on the source branch, including commits you push after you create the pull request. Since you are making all your fixes directly on your develop branch, this pull request now has accumulated fixes for three unrelated issues, which makes review more difficult.

Pull requests are designed for a workflow where you make your fixes on individual bugfix/feature branches in your repo rather than the main development branch; then the pull request will only ever include the one thing (or any updates you make to that specific bugfix/feature branch, which should all be related to the same issue).

@carljm
Copy link
Contributor

carljm commented Dec 12, 2011

The other risk with making changes directly in your develop branch is that your changes may not be merged as-is. In this case, the fix for #70 is wrong (I'll explain in a comment on that issue), but the fixes for #182 and #310 are fine. Since you committed them all on the same branch, and the wrong fix first, I have to cherry-pick instead of merge, meaning you'll now have to force-reset your develop branch.

Thanks for the fixes!

@carljm carljm closed this Dec 12, 2011
@ogirardot
Copy link
Contributor Author

oops :)
yeah sorry i got carried away.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants