FutureDatetime and PastDatetime providers #779
Conversation
|
@ceccoemi I see you have requested a review, but as I can judge there is only one method implemented in the provider. Did you request a review to illustrate the idea? |
@lk-geimfari yes exactly, I don't want to write a big amount of code and then discover that you don't like my solutions. The other methods will be implemented with the same approach. |
|
@ceccoemi I agree. It's better to discuss solutions. Well, it looks good for me. @sobolevn What do you think? Any comments? |
sinecode
changed the title
Codecov Report
@@ Coverage Diff @@
## master #779 +/- ##
==========================================
+ Coverage 98.54% 98.83% +0.29%
==========================================
Files 60 60
Lines 1919 1971 +52
==========================================
+ Hits 1891 1948 +57
+ Misses 28 23 -5 |
mimesis/providers/date.py
Outdated
| self._dt = Datetime(*args, **kwargs) | ||
| self.day_of_week = self._dt.day_of_week | ||
| self.month = self._dt.month | ||
| self.century = self._dt.century | ||
| self.periodicity = self._dt.periodicity | ||
| self.time = self._dt.time | ||
| self.formatted_time = self._dt.formatted_time | ||
| self.day_of_month = self._dt.day_of_month | ||
| self.timezone = self._dt.timezone | ||
| self.gmt_offset = self._dt.gmt_offset | ||
| self.timestamp = self._dt.timestamp |
There was a problem hiding this comment.
@lk-geimfari @sobolevn I had to switch to composition instead of inheritance because of the incompatibility with the overloaded methods.
I know this section is a bit tricky. It can be replaced with:
self._dt = Datetime(*args, **kwargs)
for method in (
m for m in dir(Datetime)
if callable(getattr(Datetime, m))
and not m.startswith('_')
and m not in dir(FutureDatetime)
):
setattr(self, method, getattr(self._dt, method))
which it automatically adds the methods not overridden, but it is more black magic and less explicit. What do you prefer?
There was a problem hiding this comment.
@ceccoemi I think that we need to implement only some methods in this provider, not all of them. We must decide which methods can be really useful in FutureDatetime.
There was a problem hiding this comment.
@lk-geimfari ok, if you don't want all the methods then it's really easy.
I propose to keep date, formatted_date, datetime, formatted_datetime and timestamp only.
There was a problem hiding this comment.
@lk-geimfari see my last commit!
There was a problem hiding this comment.
@ceccoemi I agree with your propose.
|
@lk-geimfari please wait before publish a new version. There are some sections in the documentation that must be updated. I will do it as soon as this PR is merged in another branch. |
|
@ceccoemi Although I completely like your solution, I'm not quite sure that this idea was a good one, when now I see how much code of the same type we got because of the specifics of the issue. If you do not mind I'll try to figure out how we can do it in a much simple and clear way. Can you, please, freeze your activity on this PR for now? You can work on another issue if you want. |
|
@ceccoemi Sorry for this. I hope that you're not mad. |
|
@lk-geimfari ok, no problem. |
|
@lk-geimfari I agree that there is a lot of repetition. Maybe the best thing to do is to implement just some methods in the |
|
@ceccoemi We need to check out how the faker implements similar features and implement this feature in a similar way. Related: https://faker.readthedocs.io/en/master/providers/faker.providers.date_time.html |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Closing this pull request automatically because it has not had any activity since it has been marked as stale. If you think it is still relevant and should be addressed, feel free to open a new one. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Closing this pull request automatically because it has not had any activity since it has been marked as stale. If you think it is still relevant and should be addressed, feel free to open a new one. |
I have made things!
Checklist
Related issues
Format is:
@lk-geimfari
@sobolevn
Before I continue any further, I'd like to know what do you think about this approach.
FutureDatetimeis a subclass ofDatetime. It takes some argument in the__init__()function to specify how much time to shift from the time when it's initialized.All the methods that take as argument a
startdate are overrided andstartis replaced with the current future. Theendargument is set in a way that the timedelta fromstarttoendis the same inDatetimeand inFutureDatetime. For example inDatetime.week_date(),start=2017andend=2018, there is a year of difference in the default arguments, so inFutureDatetime, by default, we havestart=self.future.yearandend=self.future.year+1.