Fix on gcalendar module

tobi-wan-kenobi / bumblebee-status

bumblebee-status is a modular, theme-able status line generator for the i3 window manager.

https://bumblebee-status.readthedocs.io/en/main/

MIT License

1.22k stars 228 forks source link

Fix on gcalendar module #911

Closed diesphink closed 2 years ago

diesphink commented 2 years ago

Fixed save/load of token (using pickle, the other method returned error on creds.to_json() as invalid)
Fixed for multiple calendars (was returning no event if the first calendar had no events)

tobi-wan-kenobi commented 2 years ago

Thank you for your contribution!

I have a question: Can we maybe store the credentials directly as binary file? Seems to me that would be easier and remove the pickle dependency.

Also, reg. regolith: The change looks good, but maybe it would be more general to use the parameter to determine a separator and use that? (I can gladly do that myself post-merge)

What do you think?

diesphink commented 2 years ago

Hi.

I have a question: Can we maybe store the credentials directly as binary file? Seems to me that would be easier and remove the pickle dependency.

Sure, I just don't know a reliable way to do it, pickle is already binary and is my go-to solution for those scenarios, but feel free to change anything about this.

Also, reg. regolith: The change looks good, but maybe it would be more general to use the parameter to determine a separator and use that? (I can gladly do that myself post-merge)

Sure, just a reminder: it's the separator and also the char at the end of command sent to socket. I have no idea why the hell they changed the communication protocol on the fork.

If you make those parameters more generic, I'd also comment on the description the variation needed for regolith-rofication, because when I loaded the module it just froze the bar completely because it's waiting on the comm.

diesphink commented 2 years ago

OH, sorry! Now I see that I've made this PR with master, instead of a branch, my mistake. That's why it duplicated the other PR.

Also, I've unintentionally added another commit with support for locale on the gcalendar

tobi-wan-kenobi commented 2 years ago

No worries, happens :D

Do you prefer cleaning up this PR, or would you rather close this one and create a fresh one?

diesphink commented 2 years ago

Well, the commit was another fix, to use default locale on gcalendar, and also support to set a different locale, so if your inclined to merge the other fixes, I would add this one as well.

tobi-wan-kenobi commented 2 years ago

Good for me as well!

tobi-wan-kenobi commented 2 years ago

OK, so, I was basically almost merging this now, but then something occurred to me: The change from JSON to pickle is problematic to me, for 2 reasons:

(minor) I do not get why this is a problem. The Google API seems to offer a "credential to JSON" conversion, and that seems to fail - which is unexpected.
(major) How is backwards-compatibility? I.e. if somebody uses this module & has credentials stored as JSON already, how will this change affect them? Will this "auto-heal", or do they have to manually delete the credential file?

Once 2. is properly addressed, I will gladly merge the PR.

Thank you kindly for your effort!

diesphink commented 2 years ago

Fair point, let me see if I can make it use the same json format as before.

diesphink commented 2 years ago

Ok, reverted the adjustment, the things was: the distro package for python3-googleapi was 1.7.11-4, to_json is available from 1.8.0. Added a comment about that on the dependencies.

tobi-wan-kenobi commented 2 years ago

Great, thank you kindly!

tobi-wan-kenobi commented 2 years ago

Changes look good to me, thank you very much!