-
-
Notifications
You must be signed in to change notification settings - Fork 10
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
functions_date.py #94
Conversation
Reviewer's Guide by SourceryThis pull request updates the File-Level Changes
Tips
|
for more information, see https://pre-commit.ci
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.
Hey @ufoptg - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Potential hard-coded secret found in data dictionary. (link)
- Potential hard-coded secret found in data dictionary. (link)
Overall Comments:
- Consider simplifying the polynomial fitting to a linear regression unless there's a specific need for higher-order polynomials. If keeping the current approach, please provide justification for the chosen polynomial order.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🔴 Security: 2 blocking issues
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
def __init__(self, order = 3): | ||
self.order = 3 | ||
|
||
def __init__(self, order=3): |
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.
suggestion: Consider adding a maximum limit to the polynomial order
While allowing variable order polynomials provides flexibility, high-order polynomials can lead to overfitting and numerical instability. Consider adding an upper bound to prevent potential issues.
def __init__(self, order=3, max_order=10):
self.order = min(order, max_order)
@@ -11,6 +11,8 @@ | |||
from datetime import datetime | |||
|
|||
data = { | |||
"7117444122": 1723429195, |
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.
🚨 issue (security): Potential hard-coded secret found in data dictionary.
The value '7117444122' appears to be a hard-coded identifier or key. Please ensure this is not a sensitive or production secret.
@@ -11,6 +11,8 @@ | |||
from datetime import datetime | |||
|
|||
data = { | |||
"7117444122": 1723429195, | |||
"6602485156": 1723148395, |
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.
🚨 issue (security): Potential hard-coded secret found in data dictionary.
The value '6602485156' appears to be a hard-coded identifier or key. Please ensure this is not a sensitive or production secret.
Summary by Sourcery
Enhance the UserDateEstimator class by adding a check for existing
tg_id
values in the data and improving thetime_format
method to provide more detailed output. Add new data entries to the dataset.Enhancements:
func
method to return the exact value if thetg_id
exists in the data.time_format
method to return both the formatted date and the age in years, months, and days.