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

reddit: updating reddit module praw dependency #1505

Merged
merged 1 commit into from
May 27, 2019

Conversation

RustyBower
Copy link
Contributor

  • version locking praw
  • updating code for new praw version
  • narrowing exception

sopel/modules/reddit.py Outdated Show resolved Hide resolved
@dgw dgw added this to the 7.0.0 milestone Mar 16, 2019
@dgw dgw added the Tweak label Mar 16, 2019
@dgw
Copy link
Member

dgw commented Mar 16, 2019

Want to fix the style checker failure? I probably won't have a chance to look at this properly until tomorrow, though, so if you want to minimize amends/force-pushes, you can also just wait. :)

@dgw dgw self-requested a review March 18, 2019 04:21
@dgw
Copy link
Member

dgw commented Apr 15, 2019

Needs rebasing due to updated requirements for spellcheck.

Also needs tweaking to avoid undoing changes made in #1516.

@Exirel
Copy link
Contributor

Exirel commented May 8, 2019

@RustyBower be careful with that PR, this should be probably rebased once both #1486 and #1503 are merged. Still OK to work on that? 😃

@dgw
Copy link
Member

dgw commented May 12, 2019

@Exirel A "High Priority" PR blocked on a "Low Priority" one (#1503), lol. 😁

@RustyBower
Copy link
Contributor Author

This should be rebased and fixed now

Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

I found it very helpful to review this with whitespace-only changes ignored. Doing so let me very quickly catch places where another PR (#1516) was partially reverted.

sopel/modules/reddit.py Outdated Show resolved Hide resolved
sopel/modules/reddit.py Outdated Show resolved Hide resolved
sopel/modules/reddit.py Outdated Show resolved Hide resolved
sopel/modules/reddit.py Outdated Show resolved Hide resolved
sopel/modules/reddit.py Outdated Show resolved Hide resolved
sopel/modules/reddit.py Outdated Show resolved Hide resolved
@RustyBower RustyBower force-pushed the reddit_fix_7 branch 2 times, most recently from d97686e to 7977a8e Compare May 13, 2019 05:57
@dgw
Copy link
Member

dgw commented May 23, 2019

Blahhh, I kinda promised to merge this before #1486 and now I went and merged that first, creating a conflict with this PR. Ugh. I got trigger-happy. 🙏

@RustyBower
Copy link
Contributor Author

All rebased

Exirel
Exirel previously approved these changes May 23, 2019
Copy link
Contributor

@Exirel Exirel left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

There's some major duplication in this after rebase.

message = '[REDDITOR] ' + u.name
now = dt.datetime.utcnow()
cakeday_start = dt.datetime.utcfromtimestamp(u.created_utc)
cakeday_start = cakeday_start.replace(year=now.year)
day = dt.timedelta(days=1)
year_div_by_400 = now.year % 400 == 0
year_div_by_100 = now.year % 100 == 0
year_div_by_4 = now.year % 4 == 0
is_leap = year_div_by_400 or ((not year_div_by_100) and year_div_by_4)
if (not is_leap) and ((cakeday_start.month, cakeday_start.day) == (2, 29)):
# If cake day is 2/29 and it's not a leap year, cake day is 1/3.
# Cake day begins at exact account creation time.
is_cakeday = cakeday_start + day <= now <= cakeday_start + (2 * day)
else:
is_cakeday = cakeday_start <= now <= cakeday_start + day
if is_cakeday:
message = message + ' | ' + bold(color('Cake day', colors.LIGHT_PURPLE))
if commanded:
message = message + ' | https://reddit.com/u/' + u.name
if u.is_gold:
message = message + ' | ' + bold(color('Gold', colors.YELLOW))
if u.is_mod:
message = message + ' | ' + bold(color('Mod', colors.GREEN))
message = message + (' | Link: ' + str(u.link_karma) +
' | Comment: ' + str(u.comment_karma))
bot.say(message)

duplicates the logic outside that try block, just below:

message = '[REDDITOR] ' + u.name
now = dt.datetime.utcnow()
cakeday_start = dt.datetime.utcfromtimestamp(u.created_utc)
cakeday_start = cakeday_start.replace(year=now.year)
day = dt.timedelta(days=1)
year_div_by_400 = now.year % 400 == 0
year_div_by_100 = now.year % 100 == 0
year_div_by_4 = now.year % 4 == 0
is_leap = year_div_by_400 or ((not year_div_by_100) and year_div_by_4)
if (not is_leap) and ((cakeday_start.month, cakeday_start.day) == (2, 29)):
# If cake day is 2/29 and it's not a leap year, cake day is 3/1.
# Cake day begins at exact account creation time.
is_cakeday = cakeday_start + day <= now <= cakeday_start + (2 * day)
else:
is_cakeday = cakeday_start <= now <= cakeday_start + day
if is_cakeday:
message = message + ' | ' + bold(color('Cake day', colors.LIGHT_PURPLE))
if commanded:
message = message + ' | https://reddit.com/u/' + u.name
if u.is_gold:
message = message + ' | ' + bold(color('Gold', colors.YELLOW))
if u.is_mod:
message = message + ' | ' + bold(color('Mod', colors.GREEN))
message = message + (' | Link: ' + str(u.link_karma) +
' | Comment: ' + str(u.comment_karma))
bot.say(message)

Not sure what happened @RustyBower but please deduplicate and make sure you don't undo any of #1486 (which touched a bit of stuff, including making sure that "2/29" and "3/1" in the cake-day comment match—one of your comments is still the old "2/29" and "1/3" mismatched one).

sopel/modules/reddit.py Outdated Show resolved Hide resolved
sopel/modules/reddit.py Outdated Show resolved Hide resolved
sopel/modules/reddit.py Outdated Show resolved Hide resolved
@dgw dgw dismissed Exirel’s stale review May 24, 2019 02:57

sprays @Exirel with water bad kitty, review more carefully!

@RustyBower RustyBower force-pushed the reddit_fix_7 branch 3 times, most recently from 7669879 to cf7284d Compare May 24, 2019 03:28
Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

I'd say this is one more nitpicking review for good measure, but this isn't a nitpick… it's a bug that will break handling reddit links unless it's fixed. 😆

sopel/modules/reddit.py Outdated Show resolved Hide resolved
Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

Tested and found a problem that appears unrelated to any of these changes. Giving it the go-ahead while I turn my attention to figuring out the other issue.

@dgw dgw requested a review from Exirel May 24, 2019 05:08
@dgw dgw mentioned this pull request May 24, 2019
Copy link
Contributor

@Exirel Exirel left a comment

Choose a reason for hiding this comment

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

AFAICT, 🚢

@dgw dgw merged commit c6c47d4 into sopel-irc:master May 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants