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

172 holidays and weekends dont extend late calculation #219

Merged
merged 13 commits into from
Mar 13, 2024

Conversation

frozenfrank
Copy link
Contributor

This resolves #172 with a very robust capability of reading configuration strings. This does not set up the dynamic string configuration since it needs to be decided on a higher level. That issue is contemplated in #156.

This introduces the ability to respond to configuration values in two different formats:

  • single line strings, optimized for storing in a cell of a configuration table which can be updated
  • multi-line strings, optimized for storing in a file with readable comments and understandable language

Examples of both formats are tested in appropriate test files.

While this isn't primarily concerned with this, we do introduce hard-coded settings to carry us through the 2024 calendar year. Some dates may be incorrect; rather than adjusting them here, we should choose a method for storing them more dynamically which this PR supports.

Credit to @prattnj for building the original pattern from which we have added this robust functionality. Thanks to @19mdavenport for support in creating the appropriate test files and determine the correct behavior.

@frozenfrank frozenfrank added the enhancement New feature or request label Mar 11, 2024
@frozenfrank frozenfrank requested a review from pawlh March 11, 2024 19:43
@frozenfrank frozenfrank self-assigned this Mar 11, 2024
@frozenfrank frozenfrank marked this pull request as draft March 11, 2024 19:47
@pawlh
Copy link
Collaborator

pawlh commented Mar 11, 2024

Woops, sorry I did not intend to add switch back to cache action to your PR, although there shouldn't be a problem if it stays here

@frozenfrank
Copy link
Contributor Author

Woops, sorry I did not intend to add switch back to cache action to your PR, although there shouldn't be a problem if it stays here

@pawlh That's ok. Since I see you already have it on another branch as well, I'll remove it.

I am going to add one more test before I unmark this as a "draft." I forgot to validate that the due dates actually extend properly when the holidays are configured!

@frozenfrank frozenfrank force-pushed the 172-holidays-and-weekends-dont-extend-late-calculation branch from 3fb2dc3 to d23eaf4 Compare March 11, 2024 19:50
- Create shared functions to reduce code duplication
- Create multiple new test cases to document how the function should behave

NOTE that associated visualizations of the test cases are included with the PR.
@frozenfrank frozenfrank marked this pull request as ready for review March 11, 2024 21:27
@frozenfrank
Copy link
Contributor Author

@pawlh This is now ready for review

@frozenfrank
Copy link
Contributor Author

@leesjensen You may appreciate thoroughness of the testing of this method. Because of the test cases evaluated, I am now very confident that this works as it is supposed to work. Hopefully, this will be appreciated as a way of future proofing this code and making it more accessible to future TA's.

Copy link
Contributor Author

@frozenfrank frozenfrank left a comment

Choose a reason for hiding this comment

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

I feel like I did a great job :)

…fore using them.

This loud check will warn developers before they have an attempt to use the code improperly.
@frozenfrank frozenfrank force-pushed the 172-holidays-and-weekends-dont-extend-late-calculation branch from 6efd2bc to edb79e5 Compare March 11, 2024 23:21
@frozenfrank
Copy link
Contributor Author

For good measure, a screenshot of all the tests passing:

image

@pawlh pawlh merged commit e1c4754 into main Mar 13, 2024
1 check passed
@pawlh pawlh deleted the 172-holidays-and-weekends-dont-extend-late-calculation branch March 13, 2024 01:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

backend: Holidays and weekends don't unnecessarily extend the late penalty calculation
2 participants