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

Always use config to set default value of timeout in check version #2108

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sunnyakaxd
Copy link
Contributor

@sunnyakaxd sunnyakaxd commented Aug 22, 2020

This change is Reviewable

Copy link
Contributor

@jeffwidman jeffwidman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little torn on this one... throughout the code there are a bunch of places we explicitly pass in the config value, and then here we fall back to the config as a default... if we go this route, then we should do it completely and not explicitly pass it in elsewhere and just let them fallback to this default...

Also, there's another default of 2 in conn.check_version() which should probably also default to this value as well for consistency. (although it doesn't look like the default value is ever really used as the only places its called have a passed-in value)

I'd like to hear input though from @dpkp / @tvoinarovskyi on whether they'd rather we get to this value by default or by explicitly passing it in everywhere

@dpkp dpkp force-pushed the master branch 2 times, most recently from 9c8c8af to 8ebb14c Compare February 12, 2025 22:19
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