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 flag support #92

Merged
merged 73 commits into from
Nov 20, 2022

Conversation

thehesiod
Copy link
Contributor

@thehesiod thehesiod commented Jul 12, 2018

adds Client arg for method to decode values which have flags

@codecov
Copy link

codecov bot commented Jul 12, 2018

Codecov Report

Merging #92 into master will increase coverage by 0.8%.
The diff coverage is 93.33%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master     #92     +/-   ##
========================================
+ Coverage    91.5%   92.3%   +0.8%     
========================================
  Files           5       6      +1     
  Lines         259     286     +27     
  Branches       38      44      +6     
========================================
+ Hits          237     264     +27     
  Misses         11      11             
  Partials       11      11
Impacted Files Coverage Δ
aiomcache/__init__.py 100% <ø> (ø) ⬆️
aiomcache/helpers.py 100% <100%> (ø)
aiomcache/client.py 88.65% <77.77%> (+0.36%) ⬆️

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 692e2c0...53699a3. Read the comment docs.

@thehesiod
Copy link
Contributor Author

btw another option is adding a client wrapper what will do the right thing

@Dreamsorcerer
Copy link
Member

I'm not so familiar with these flags. But, would you still be interested in updating the PR to get this merged?

@thehesiod
Copy link
Contributor Author

rebasing now

jayzfbn and others added 3 commits November 10, 2022 15:45
attempt to fix socket warnings in test, remove direct pylibmc dependency
@lgtm-com
Copy link

lgtm-com bot commented Nov 11, 2022

This pull request introduces 1 alert when merging 65686d3 into 56d1377 - view on LGTM.com

new alerts:

  • 1 for Unused import

@Dreamsorcerer
Copy link
Member

Separately, the unclosed socket errors I was able to duplicate locally and they seem to be addressed by adding some connection release/cleanup to the other tests. Re: 294

Got a PR for that?

@jayzfbn
Copy link
Contributor

jayzfbn commented Nov 11, 2022

| Got a PR for that?

It's in this PR. I'm off today but I'll sit down and take another spin at this on Monday and try to address the remaining comments.

@Dreamsorcerer
Copy link
Member

| Got a PR for that?

It's in this PR. I'm off today but I'll sit down and take another spin at this on Monday and try to address the remaining comments.

Right, I missed the first set of commits. Weird that the failing test is not the one that got updated (or even the before the failing test)...

@aio-libs aio-libs deleted a comment from CLAassistant Nov 11, 2022
@jayzfbn
Copy link
Contributor

jayzfbn commented Nov 14, 2022

I think my last commit should address the open comments. I also added some tests just for my own sanity.

@lgtm-com
Copy link

lgtm-com bot commented Nov 14, 2022

This pull request introduces 1 alert when merging 6d24183 into 2aac5e5 - view on LGTM.com

new alerts:

  • 1 for Unused import

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. It looks like GitHub code scanning with CodeQL is already set up for this repo, so no further action is needed 🚀. For more information, please check out our post on the GitHub blog.

@lgtm-com
Copy link

lgtm-com bot commented Nov 20, 2022

This pull request introduces 1 alert when merging c04c6d2 into cc2a55a - view on LGTM.com

new alerts:

  • 1 for Unused import

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. It looks like GitHub code scanning with CodeQL is already set up for this repo, so no further action is needed 🚀. For more information, please check out our post on the GitHub blog.

@lgtm-com
Copy link

lgtm-com bot commented Nov 20, 2022

This pull request introduces 1 alert when merging 36cb848 into cc2a55a - view on LGTM.com

new alerts:

  • 1 for Unused import

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. It looks like GitHub code scanning with CodeQL is already set up for this repo, so no further action is needed 🚀. For more information, please check out our post on the GitHub blog.

@Dreamsorcerer Dreamsorcerer merged commit 0423669 into aio-libs:master Nov 20, 2022
@Dreamsorcerer
Copy link
Member

Thanks for that, finally got through it all.

@jayzfbn
Copy link
Contributor

jayzfbn commented Dec 5, 2022

Thanks a bunch for the merge; no urgency on any of this but is a release version rev on the radar any time soon? Totally fine waiting a bit if one is coming sometime soon anyway, just thought I would check. Thx!

@Dreamsorcerer
Copy link
Member

Yep, I'll do it this week. Was hoping someone might verify and fix #111 before I did a release.

@jayzfbn
Copy link
Contributor

jayzfbn commented Dec 5, 2022

No rush; thanks a bunch!

@Dreamsorcerer
Copy link
Member

Will try a release on Sunday.

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.

3 participants