Skip to content

revert bump to markdownify version, ensure redis cache ttl is set correctly#2156

Merged
HassanAbouelela merged 9 commits into
python-discord:mainfrom
Numerlor:revert-markdownify-bump
May 27, 2022
Merged

revert bump to markdownify version, ensure redis cache ttl is set correctly#2156
HassanAbouelela merged 9 commits into
python-discord:mainfrom
Numerlor:revert-markdownify-bump

Conversation

@Numerlor
Copy link
Copy Markdown
Contributor

@Numerlor Numerlor commented Apr 29, 2022

The new versions bumped in bd13ed6 introduce conversions which cause the doc embeds to be formatted improperly, e.g.

new markdownify old markdownify

I've also removed an invalid task cancel in 4cdaaf4, and ensured that an expire is set after a cache clear in df99fa8

The new versions introduce conversions which causes the doc command
embed to be formatted improperly
@Numerlor Numerlor requested review from Den4200 and jb3 as code owners April 29, 2022 14:02
Numerlor added 2 commits April 29, 2022 16:31
if the expire "cache" is not reset, the class assumes an expire is set,
even though no expire was set for the key
The task is no longer created in the cog
@Numerlor Numerlor force-pushed the revert-markdownify-bump branch from a5561be to 4cdaaf4 Compare April 29, 2022 14:34
@ChrisLovering ChrisLovering added a: frontend Related to output and formatting p: 1 - high High Priority t: enhancement Changes or improvements to existing features status: approved The issue has received a core developer's approval s: needs review Author is waiting for someone to review and approve labels Apr 29, 2022
@Numerlor Numerlor changed the title revert bump to markdownify version revert bump to markdownify version, ensure redis cache ttl is set correctly Apr 29, 2022
Copy link
Copy Markdown
Contributor

@wookie184 wookie184 left a comment

Choose a reason for hiding this comment

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

This seems alright but i'm not 100% sure on the changes to the set function. I may just be misunderstanding though.

If set was called after a bot restart with an hour until a key expired, the expiry would not be changed but the _set_expires value would be set to a week. If set was called a day later, after the key had expired, the key would be set but no expiry would be set. I'm not sure if this would happen with the current workings of doc but if it didn't i'm not sure how the changes would help.

Would a more robust change be to replace needs_expire = not await connection.exists(redis_key) with checking the ttl is greater than 0 https://redis.io/commands/ttl/ (so we catch keys that didn't exist before and also keys with no expiry set). Then we can put that expiry in _set_expires, which seems safer than assuming it's just a week from now. If it wasn't set we can set the expiry and also set it in _set_expires.

Comment thread bot/exts/info/doc/_redis_cache.py Outdated
Comment thread bot/exts/info/doc/_redis_cache.py Outdated
@Numerlor
Copy link
Copy Markdown
Contributor Author

Thanks, I forgot to take into account the ongoing expires in redis, should be fine now I believe

Copy link
Copy Markdown
Contributor

@wookie184 wookie184 left a comment

Choose a reason for hiding this comment

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

One thing, other than that all looks good.

Comment thread bot/exts/info/doc/_redis_cache.py Outdated
Copy link
Copy Markdown
Contributor

@wookie184 wookie184 left a comment

Choose a reason for hiding this comment

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

All seems good to me. Thanks

@HassanAbouelela
Copy link
Copy Markdown
Contributor

For the record, I'm highly opposed to the philosophy of not updating our deps because we can, however that's not a reason to block the PR for now since it'll fix the bug. We can work on getting it working in another PR. I suspect no one will pick that up though, so I'll try to get it to it once I have some free time.

@HassanAbouelela HassanAbouelela enabled auto-merge May 20, 2022 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: frontend Related to output and formatting p: 1 - high High Priority s: needs review Author is waiting for someone to review and approve status: approved The issue has received a core developer's approval t: enhancement Changes or improvements to existing features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants