ocf / ocflib

Python libraries for account and server management
https://pypi.python.org/pypi/ocflib
Other
15 stars 32 forks source link

adding semester_dates function #58

Closed shex1627 closed 7 years ago

shex1627 commented 7 years ago

A utility function for the semester job distribution chart.

1.I thought of making spring_start and fall_start constants but "year" would be a variable that later I have to fix, which I think will make the code harder to understand unless you guys have some better solutions.

shex1627 commented 7 years ago

So the logic is: from 01/01 to 07/31 is considered "spring" and 08/01 to 12/31 is considered "fall"

Basically you have one day, and you expand that day to the semester start and semester end. Subtracting one is to make sure the fall and spring don't overlap.

abizer commented 7 years ago

Oh ok that makes sense

On Mar 12, 2017 2:17 PM, "shicheng" notifications@github.com wrote:

So the logic is: from 01/01 to 07/31 is considered "spring" and 08/01 to 12/31 is considered "fall"

Basically you have one day, and you expand that day to the semester start and semester end. Subtracting one is to make sure the fall and spring don't overlap.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/ocf/ocflib/pull/58#issuecomment-285977435, or mute the thread https://github.com/notifications/unsubscribe-auth/AB0_lqyrLeQh5yWtSZuRWlKtcDQLUW4Yks5rlGDvgaJpZM4Map3k .

matthew-mcallister commented 7 years ago

Also, can you add a simple test for this? Not really sure what would be good. Maybe just pick a couple dates that you think should be in the same semester and test that they return the same tuple?

shex1627 commented 7 years ago

commented so that you guys can see the new commit.

kpengboy commented 7 years ago

While you're at it, you can rewrite the current_semester_start() function to call semester_dates instead.

shex1627 commented 7 years ago

New revision updated. Why is Kevin so good at detecting logical bugs?

shex1627 commented 7 years ago

could anyone have a look at this?

shex1627 commented 7 years ago

@chriskuehl I use your functool solution, but also rename constants to be like FALL_START_FUN so that others know they are not date objects when they want to use them.

abizer commented 7 years ago

rename them entirely to something more indicative of being functions like get_fall_start or something like that?

On Apr 30, 2017 3:23 PM, "shicheng" notifications@github.com wrote:

@shex1627 commented on this pull request.

In ocflib/lab/stats.py https://github.com/ocf/ocflib/pull/58#discussion_r114085819:

@@ -13,6 +14,12 @@

when we started keeping stats

STATS_EPOCH = date(2014, 2, 15)

+# function partial constants for semesters dates +SPRING_START_FUNP = functools.partial(date, month=1, day=1) +SPRING_END_FUNP = functools.partial(date, month=7, day=31) +FALL_START_FUNP = functools.partial(date, month=8, day=1) +FALL_END_FUNP = functools.partial(date, month=12, day=31)

although those are functions but I think the names could be misleading for people to think they are date objects. Do we need to do anything about that? Or You don't think it is a problem at all.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/ocf/ocflib/pull/58#discussion_r114085819, or mute the thread https://github.com/notifications/unsubscribe-auth/AB0_ltiKENcLW86oqs5kvzm_M2APC6B8ks5r1QnpgaJpZM4Map3k .

shex1627 commented 7 years ago

renaming all those function partials to get_xxx_xxx, so are we ready to merge now?

shex1627 commented 7 years ago

adding prefix "_" to the functions

shex1627 commented 7 years ago

do you guys want me to squash the commits as well

matthew-mcallister commented 7 years ago

Oh, it looks like the squash and merge button isn't available. You can if you want, otherwise I will.

shex1627 commented 7 years ago

I will mess with my local repo see if I can do it myself first. Can't rely on others every time

shex1627 commented 7 years ago

Seems like I did it(squash the commits) guys Not sure if the standard way first git reset --soft HEAD~9 && git commit git push --force origin semesterJobUtil

matthew-mcallister commented 7 years ago

shipit