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

Add support for TS.MGET with options. minor refactor and go fmt. #67

Merged
merged 2 commits into from
Jul 19, 2020

Conversation

filipecosta90
Copy link
Collaborator

This PR adresses #64 .
It also enforces go fmt to pass the PR.

@filipecosta90 filipecosta90 added the enhancement New feature or request label Jul 18, 2020
@codecov
Copy link

codecov bot commented Jul 18, 2020

Codecov Report

Merging #67 into master will increase coverage by 0.56%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #67      +/-   ##
==========================================
+ Coverage   81.09%   81.66%   +0.56%     
==========================================
  Files           6        7       +1     
  Lines         455      469      +14     
==========================================
+ Hits          369      383      +14     
  Misses         46       46              
  Partials       40       40              
Impacted Files Coverage Δ
pool.go 80.43% <ø> (ø)
reply_parser.go 67.92% <ø> (ø)
client.go 87.35% <100.00%> (ø)
multiget.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2169513...5298f85. Read the comment docs.

@filipecosta90
Copy link
Collaborator Author

cc @stevesim101

@filipecosta90 filipecosta90 self-assigned this Jul 19, 2020

.PHONY: all test coverage
all: test coverage

checkfmt:
Copy link

Choose a reason for hiding this comment

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

@filipecosta90 While this will work, an easier way of linting in go would be using https://github.com/golangci/golangci-lint

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@danni-m I've used this go fmt approach as a quick fix/quick enforcing of fmt. Will revise the required changes to use golangci-lint and move towards that afterwards. wdyt of adding this quick check to be safeguarded and with more time moving to something more correct/sophisticated?

Copy link

Choose a reason for hiding this comment

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

@filipecosta90 sounds good to me, just want to make sure you are aware that this tool exists.

client.go Show resolved Hide resolved
@danni-m
Copy link

danni-m commented Jul 19, 2020

@filipecosta90 Im think its pretty good, I would reconsider having 2 functions.

@filipecosta90
Copy link
Collaborator Author

@filipecosta90 Im think its pretty good, I would reconsider having 2 functions.

@danni-m I believe that for being backwards compatible we need them both. Notwithstanding followed your advise and merge the internals of both. wdyt?

@filipecosta90 filipecosta90 merged commit 3f23ae8 into master Jul 19, 2020
@filipecosta90 filipecosta90 deleted the multi.get.with.options branch July 20, 2020 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants