Skip to content

Conversation

@sinisaos
Copy link
Member

@sinisaos sinisaos commented Aug 6, 2022

@dantownsend I hope you meant something similar to this. Once you've checked and merged this, I'll make the changes in Piccolo Admin. Related to this issue.

@codecov-commenter
Copy link

codecov-commenter commented Aug 6, 2022

Codecov Report

Merging #168 (4621d1c) into master (33ca725) will increase coverage by 3.66%.
The diff coverage is 97.99%.

@@            Coverage Diff             @@
##           master     #168      +/-   ##
==========================================
+ Coverage   88.38%   92.04%   +3.66%     
==========================================
  Files          29       32       +3     
  Lines        1576     1761     +185     
==========================================
+ Hits         1393     1621     +228     
+ Misses        183      140      -43     
Impacted Files Coverage Δ
piccolo_api/crud/endpoints.py 94.96% <50.00%> (-0.22%) ⬇️
piccolo_api/media/base.py 96.38% <96.38%> (ø)
piccolo_api/media/s3.py 98.88% <98.88%> (ø)
piccolo_api/media/local.py 100.00% <100.00%> (ø)
piccolo_api/crud/validators.py 93.61% <0.00%> (-0.14%) ⬇️
piccolo_api/shared/middleware/junction.py 0.00% <0.00%> (ø)
piccolo_api/jwt_auth/middleware.py 88.37% <0.00%> (+3.26%) ⬆️
piccolo_api/jwt_auth/endpoints.py 100.00% <0.00%> (+9.09%) ⬆️
piccolo_api/session_auth/endpoints.py 85.10% <0.00%> (+9.95%) ⬆️
piccolo_api/change_password/endpoints.py 96.05% <0.00%> (+10.33%) ⬆️
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@dantownsend
Copy link
Member

@sinisaos This looks great, thanks.

I think the only thing we should consider is having boto3 as an optional requirement in setup.py.

https://github.com/piccolo-orm/piccolo_admin/blob/master/setup.py

@lgtm-com
Copy link

lgtm-com bot commented Aug 6, 2022

This pull request fixes 1 alert when merging 02bc007 into 33ca725 - view on LGTM.com

fixed alerts:

  • 1 for `__eq__` not overridden when adding attributes

@sinisaos
Copy link
Member Author

sinisaos commented Aug 6, 2022

@dantownsend I hope that's what you meant, otherwise feel free to change it.

@lgtm-com
Copy link

lgtm-com bot commented Aug 6, 2022

This pull request fixes 1 alert when merging f063e4a into 33ca725 - view on LGTM.com

fixed alerts:

  • 1 for `__eq__` not overridden when adding attributes

@lgtm-com
Copy link

lgtm-com bot commented Aug 6, 2022

This pull request introduces 1 alert and fixes 1 when merging c059447 into 33ca725 - view on LGTM.com

new alerts:

  • 1 for Unused import

fixed alerts:

  • 1 for `__eq__` not overridden when adding attributes

@sinisaos
Copy link
Member Author

sinisaos commented Aug 6, 2022

@dantownsend Can you please help me with these lgtm warnings?

@dantownsend
Copy link
Member

@sinisaos I'll have a look.

@sinisaos
Copy link
Member Author

sinisaos commented Aug 6, 2022

@dantownsend Thanks

@lgtm-com
Copy link

lgtm-com bot commented Aug 6, 2022

This pull request fixes 1 alert when merging ab9904f into 33ca725 - view on LGTM.com

fixed alerts:

  • 1 for `__eq__` not overridden when adding attributes

@lgtm-com
Copy link

lgtm-com bot commented Aug 6, 2022

This pull request fixes 1 alert when merging 4621d1c into 33ca725 - view on LGTM.com

fixed alerts:

  • 1 for `__eq__` not overridden when adding attributes

@dantownsend
Copy link
Member

dantownsend commented Aug 6, 2022

@sinisaos I think it's OK. Sometimes LGTM can be annoying. Thanks for this.

@dantownsend dantownsend merged commit ecfcac6 into piccolo-orm:master Aug 6, 2022
@sinisaos sinisaos deleted the move_mediastorage branch August 6, 2022 14:54
@dantownsend
Copy link
Member

@sinisaos I've published a new version of Piccolo API.

If you get a chance at any time, can you update Piccolo Admin, so it uses the latest version of Piccolo API, and imports this new code?

@sinisaos
Copy link
Member Author

sinisaos commented Aug 6, 2022

@dantownsend No problem. I will do it now.

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