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

feat: can resume and dump from string #5

Merged
merged 6 commits into from
Sep 27, 2023
Merged

feat: can resume and dump from string #5

merged 6 commits into from
Sep 27, 2023

Conversation

yihong0618
Copy link
Contributor

@yihong0618 yihong0618 commented Sep 26, 2023

fix: #4
@matin

Signed-off-by: yihong0618 <zouzou0208@gmail.com>
@matin
Copy link
Owner

matin commented Sep 26, 2023

My main comments:

  1. Methods should be called dumps() and loads() to stay consistent with the same pattern used elsewhere in Python
  2. base64 only makes the strings longer. Could you give it a try without that?

@matin
Copy link
Owner

matin commented Sep 26, 2023

Using base64 is ok. So, to merge:

  1. Change method names to dumps() and loads()
  2. Add tests to keep coverage at 100%

@yihong0618
Copy link
Contributor Author

yihong0618 commented Sep 26, 2023

for the reason use base64, for GitHub actions secret or other its easy to copy paste, especially for some users that do not know much about code.

for 1, its ok for me, will change it(but my question is do I need to change resume_from_string either?
for 2, will do it

@matin
Copy link
Owner

matin commented Sep 26, 2023

I would get rid of garth.resume_from_string() and garth.save_to_string() and use garth.client.loads() and garth.client.dumps() instead.

I only added save() and resume() at the top level to make it easier to read in Jupyter Notebooks, but with your use case, I wouldn't add it.

@yihong0618
Copy link
Contributor Author

OK, will fix my repo and added test tomorrow, thanks again for your great repo, and good night(good day)

Signed-off-by: yihong0618 <zouzou0208@gmail.com>
Signed-off-by: yihong0618 <zouzou0208@gmail.com>
@yihong0618
Copy link
Contributor Author

@matin done and left a few review

@@ -20,7 +19,7 @@ jobs:
strategy:
fail-fast: false
matrix:
python-version: ['3.9', '3.10', '3.11']
python-version: ["3.8", "3.9", "3.10", "3.11"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can support 3.8

Copy link
Owner

Choose a reason for hiding this comment

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

There are open issues with 3.8:

FAILED tests/stats/test_intensity_minutes.py::test_weekly_intensity_minutes - AttributeError: 'tuple' object has no attribute 'week'
FAILED tests/stats/test_steps.py::test_weekly_steps - TypeError: unsupported operand type(s) for |: 'dict' and 'dict'
FAILED tests/stats/test_stress.py::test_daily_stress - TypeError: unsupported operand type(s) for |: 'dict' and 'dict'
FAILED tests/stats/test_stress.py::test_daily_stress_pagination - TypeError: unsupported operand type(s) for |: 'dict' and 'dict'

We can make the necessary changes to support 3.8, but I recommend doing so outside of this PR.

tests/conftest.py Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 27, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (899b726) 99.79% compared to head (49c5304) 100.00%.

Additional details and impacted files
@@             Coverage Diff             @@
##             main        #5      +/-   ##
===========================================
+ Coverage   99.79%   100.00%   +0.20%     
===========================================
  Files          29        29              
  Lines         980       996      +16     
===========================================
+ Hits          978       996      +18     
+ Misses          2         0       -2     
Flag Coverage Δ
unittests 100.00% <100.00%> (+0.20%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
garth/__init__.py 100.00% <ø> (ø)
garth/http.py 100.00% <100.00%> (ø)
tests/conftest.py 100.00% <100.00%> (+2.46%) ⬆️
tests/test_http.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yihong0618
Copy link
Contributor Author

yihong0618 commented Sep 27, 2023

will fix the change in python3.8
some datetime and dict compare need fix....let me fix that.

@matin
Copy link
Owner

matin commented Sep 27, 2023

@yihong0618 with the exception of 3.8, looks great!

Could you remove 3.8 support? It can always be resolved outside of this PR.

@yihong0618
Copy link
Contributor Author

of course, dropping it

@matin matin merged commit b36caec into matin:main Sep 27, 2023
14 checks passed
@matin
Copy link
Owner

matin commented Sep 27, 2023

@yihong0618 published as version 0.4.28. Thanks for the contribution!

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.

feat: dump oauth1_token.json oauth2_token.json together, and dump them as string
2 participants