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

Adding convenience method to parse Redis version #502

Merged
merged 5 commits into from Feb 7, 2014

Conversation

Projects
None yet
3 participants
@fabsi88
Contributor

fabsi88 commented Feb 6, 2014

No description provided.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Feb 7, 2014

Member

Thanks! I don't have a Redis installation to test, so I was wondering if all lines of the info string are of the form "key: value" and if it would be safer to iterate over all lines to find the one with "redis_version" instead of assuming the second line. Or does the spec define that the version must be there anyway?

Member

s-ludwig commented Feb 7, 2014

Thanks! I don't have a Redis installation to test, so I was wondering if all lines of the info string are of the form "key: value" and if it would be safer to iterate over all lines to find the one with "redis_version" instead of assuming the second line. Or does the spec define that the version must be there anyway?

@fabsi88

This comment has been minimized.

Show comment
Hide comment
@fabsi88

fabsi88 Feb 7, 2014

Contributor

Np! Referring to the documentation of Redis (http://redis.io/commands/info) it has to be on line 2 in the output of INFO. I also thought about a solution to iterate over all lines but I think this would be some kind of overkill for the version retrievement. Another improvement would be to write a method which returns a specific attribute of the INFO - this would iterate over all lines anyway.

Contributor

fabsi88 commented Feb 7, 2014

Np! Referring to the documentation of Redis (http://redis.io/commands/info) it has to be on line 2 in the output of INFO. I also thought about a solution to iterate over all lines but I think this would be some kind of overkill for the version retrievement. Another improvement would be to write a method which returns a specific attribute of the INFO - this would iterate over all lines anyway.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Feb 7, 2014

Member

OK according to the docs, it seems like for a fully robust implementation it would be necessary to also parse the section headers. I think a good future idea would be to change the info() method to return a struct that has a string[string] map for each section instead of the raw string.

Until such a more general facility is there, though, the fixed line method should be fine. The only thing I'd change is to rename getVersion() into @property version() (it could also simply be cached after the connection has been established instead of re-requesting it for every call).

Member

s-ludwig commented Feb 7, 2014

OK according to the docs, it seems like for a fully robust implementation it would be necessary to also parse the section headers. I think a good future idea would be to change the info() method to return a struct that has a string[string] map for each section instead of the raw string.

Until such a more general facility is there, though, the fixed line method should be fine. The only thing I'd change is to rename getVersion() into @property version() (it could also simply be cached after the connection has been established instead of re-requesting it for every call).

@Extrawurst

This comment has been minimized.

Show comment
Hide comment
@Extrawurst

Extrawurst Feb 7, 2014

Contributor

@s-ludwig is it even possible to name a method "version" in D ? I just tried it does not built on win32 dmd...

Contributor

Extrawurst commented Feb 7, 2014

@s-ludwig is it even possible to name a method "version" in D ? I just tried it does not built on win32 dmd...

@Extrawurst

This comment has been minimized.

Show comment
Hide comment
@Extrawurst

Extrawurst Feb 7, 2014

Contributor

and a "Version" method would be violating the dlang style guide. I would suggest calling it redisVersion

Contributor

Extrawurst commented Feb 7, 2014

and a "Version" method would be violating the dlang style guide. I would suggest calling it redisVersion

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Feb 7, 2014

Member

Of course you are right :) The official convention would be to use version_ in this case. Maybe in this case serverVersion?

Member

s-ludwig commented Feb 7, 2014

Of course you are right :) The official convention would be to use version_ in this case. Maybe in this case serverVersion?

@Extrawurst

This comment has been minimized.

Show comment
Hide comment
@Extrawurst

Extrawurst Feb 7, 2014

Contributor

i vote in favor of redisVersion :)

Contributor

Extrawurst commented Feb 7, 2014

i vote in favor of redisVersion :)

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Feb 7, 2014

Member

Considering the original name, I change my vote to redisVersion, too ;)

Member

s-ludwig commented Feb 7, 2014

Considering the original name, I change my vote to redisVersion, too ;)

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Feb 7, 2014

Member

Okay, looks good to go! Thanks again!

(that Travic CI error can safely be ignored)

Member

s-ludwig commented Feb 7, 2014

Okay, looks good to go! Thanks again!

(that Travic CI error can safely be ignored)

s-ludwig added a commit that referenced this pull request Feb 7, 2014

Merge pull request #502 from fabsi88/master
Adding convenience method to parse Redis version

@s-ludwig s-ludwig merged commit 8fca763 into vibe-d:master Feb 7, 2014

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