-
Notifications
You must be signed in to change notification settings - Fork 0
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
PEP8-ify and change generator length calculation #3
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i like the sum(1 for ... ) ... is it demonstrably faster?
Some of the places where you suggest breaking the line into two are fine with me, but some of them are fine as is: lines 61 and 133.
Important things
- Please run the test suite... line 125 will probably break some of them.
- Please point this pull request at develop rather than master.
Thanks!
@@ -101,11 +102,13 @@ def holiday_count(start, end, skip): | |||
""" | |||
skip = skip if skip else [] | |||
non_weekend = (d for d in skip if d.weekday() < 5) | |||
return len([e for e in non_weekend if e >= start and e <= end]) | |||
# Count number of days without creating intermediate list | |||
return sum(1 for e in non_weekend if e >= start and e <= end) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
demonstrably faster?
@@ -58,7 +58,8 @@ def iterate_days(current, remaining): | |||
while guess.weekday() > 4: | |||
guess = op(guess, timedelta(days=1)) | |||
s, e = sorted([current, guess]) | |||
diff = business_days_interval(s, e, skip) - (1 if current.weekday() < 5 else 0) | |||
diff = business_days_interval(s, e, skip) - \ | |||
(1 if current.weekday() < 5 else 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't want to split this line...it's a single idea and it's not that long
d2 = dow2 if dow2 > dow1 else dow2 + 7 | ||
|
||
return len(tuple(i for i in range(dow1, d2 + 1) if i not in (5, 6))) | ||
# Count the desired number of days without creating intermediate tuples |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it turns out that this approach is faster, then we can keep the change but nix the comment.
@@ -130,9 +135,11 @@ def str_to_date(text): | |||
def main(args): | |||
|
|||
days = int(args['-n']) if args['-n'] is not None else args['-n'] | |||
start = date.today() if args['--start'] == 'today' else str_to_date(args['--start']) | |||
start = date.today() if args['--start'] == 'today' \ | |||
else str_to_date(args['--start']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope.
end = str_to_date(args['--end']) if args['--end'] is not None else None | ||
skip = tuple(str_to_date(d) for d in args['SKIP']) if args['SKIP'] is not None else args['SKIP'] | ||
skip = tuple(str_to_date(d) for d in args['SKIP']) \ | ||
if args['SKIP'] is not None else args['SKIP'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on-board with this one...
I tinkered with the
weekdays
function for a while, trying to cleverly implement some modular arithmetic but it didn't pan out. Counting the number of days elapsed, skipping Saturday and Sunday, is a good method. Theitertools.dropwhile
method would be perfect for this, if you want to go that route.