-
Notifications
You must be signed in to change notification settings - Fork 2
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
126 use full date times for dates #155
126 use full date times for dates #155
Conversation
Also improve readability of the code with named constants
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.
Let's either put back the 11:59:59 pm rule, or wait to merge this until we have confirmed that Canvas is how the professors want it
@pawlh Since you aren't actually "requesting changes," can you comment or approve the PR? There is a distinction between having the PR "approved" and actually merging it in to main. I'm on board with waiting until Canvas is updated before we merge the code as evidenced by this Slack Message. |
I'd argue that this was an appropriate use of requesting changes. In this case there were two options for changes, and action needed to be taken on one of them before I would be comfortable approving anything for merging:
Until a change was made this code would cause an undesired result. In a CI/CD situation (which I desperately wish was being used here) the Either way, action was taken and my concern is mitigated, so I'll approve this now. Thanks for doing this! |
This PR resolves #126 and also improves the quality of the code in the area directly related to that issue.
Please run and test my code before merging since I don't have a way to run the code locally.