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

Add assertion error messages #271

Merged
merged 36 commits into from
Jan 21, 2019
Merged

Add assertion error messages #271

merged 36 commits into from
Jan 21, 2019

Conversation

connorskees
Copy link
Contributor

@coveralls
Copy link

coveralls commented Jan 12, 2019

Coverage Status

Coverage increased (+0.08%) to 99.251% when pulling efca40f on ConnorSkees:master into c57abf2 on dbader:master.

@dbader
Copy link
Owner

dbader commented Jan 12, 2019

Using assert this way was a bad idea (by me). These should all be of the form

if not condition:
   raise ScheduleError(...)

Where ScheduleError is

class ScheduleValueError(ValueError):
   pass

(or a wrapper around another appropriate Python internal error class)

@connorskees
Copy link
Contributor Author

Is something like this what you were thinking?

@dbader
Copy link
Owner

dbader commented Jan 12, 2019

Is something like this what you were thinking?

Yep :)

test_schedule.py Outdated Show resolved Hide resolved
The regular expression above makes it impossible to have a number above 59 or below 0, so these exceptions can never be raised
Accidentally added when reverting
For some reason coverage dropped?
@connorskees
Copy link
Contributor Author

Did you want the assertRaises to also be changed to pytest's version? Otherwise, I believe there is nothing else to do for this pr :^)

class CancelJob(object):
"""
Can be returned from a job to unschedule itself.
"""
pass
Copy link
Owner

Choose a reason for hiding this comment

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

What happened here? Let's leave the pass in to make sure we communicate that this should be an empty class. (Same above on the various error classes, just a stylistic thing)

@dbader
Copy link
Owner

dbader commented Jan 21, 2019

Thanks! The assertRaises is fine. This looks pretty much ready, but I had one more stylistic comment (pass).

I just hope people weren't catching AssertionError in the current versions when we roll this out (:

@dbader
Copy link
Owner

dbader commented Jan 21, 2019

And please add yourself to the contributors file (if you want)

schedule/__init__.py Outdated Show resolved Hide resolved
@@ -368,16 +399,15 @@ def at(self, time_str):
second = 0
if self.unit == 'days' or self.start_day:
hour = int(hour)
assert 0 <= hour <= 23
if 0 > hour or hour > 23:
Copy link
Owner

Choose a reason for hiding this comment

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

Let's keep the condition the same for the checks. I know they're equivalent but I like 0 <= hour <= 23 better :P

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 agree!! But I couldn't figure out how to make them equivalent using that without making it
if not (0 <= hour <= 23) and I thought it was harder to read.

Do you think it should just be if not (0 <= hour <= 23) ?

Copy link
Owner

Choose a reason for hiding this comment

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

Do you think it should just be if not (0 <= hour <= 23) ?

Mmmh yeah I think that's still easier to read :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

efca40f

:^)

@dbader
Copy link
Owner

dbader commented Jan 21, 2019

Gave this another review and found some more stuff, can you comment on those?

I forgot to save the file 😧
Functionally the same, but (hopefully) more readable
@dbader
Copy link
Owner

dbader commented Jan 21, 2019

LGTM! 🎉 Nice work @connorskees!

@dbader dbader merged commit 3726589 into dbader:master Jan 21, 2019
@dbader
Copy link
Owner

dbader commented Jan 21, 2019

This is now live in https://pypi.org/project/schedule/0.6.0/

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