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

Add IsDate feature #20

Merged
merged 6 commits into from
Mar 7, 2022
Merged

Conversation

cinarizasyon
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Feb 27, 2022

Codecov Report

Merging #20 (9f71c29) into main (fe7122e) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main       #20   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            9         9           
  Lines          559       584   +25     
  Branches       145       153    +8     
=========================================
+ Hits           559       584   +25     
Impacted Files Coverage Δ
dirty_equals/__init__.py 100.00% <100.00%> (ø)
dirty_equals/_datetime.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe7122e...9f71c29. Read the comment docs.

Copy link
Owner

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

Thank you so much!

The first meaningful external contribution to this library means a lot.

Mostly LGTM, just a few things to tweak.

We should also add IsToday like IsNow, feel free to do that in this PR or I can add it seprately.

lt: Optional[date] = None,
ge: Optional[date] = None,
le: Optional[date] = None,
unix_number: bool = False,
Copy link
Owner

Choose a reason for hiding this comment

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

I think we should remove this option, it doesn't really make sense to use unix timestamps with dates.

"""

if isinstance(delta, timedelta):
delta = timedelta(days=delta.days)
Copy link
Owner

Choose a reason for hiding this comment

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

perhaps a comment here explaining why this is necessary.

"""
Args:
approx: A value to approximately compare to.
delta: The allowable different when comparing to the value to `approx`, if omitted 2 seconds is used,
Copy link
Owner

Choose a reason for hiding this comment

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

not sure 2 seconds really makes much sense here, I guess should be zero.

)

def prepare(self, other: Any) -> date:
if isinstance(other, date):
Copy link
Owner

Choose a reason for hiding this comment

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

😆 potential mistake here! datetime is a subclass of date, so isinstance(datetime.now(), date) is True.

Suggested change
if isinstance(other, date):
if type(other) == date:

Is probably simplest.

format_string: Optional[str] = None,
):

"""
Copy link
Owner

Choose a reason for hiding this comment

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

you need to add IsDate to the docs, probably add it after this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

only docs remained in todo. I'll do it in an available time.

Copy link
Owner

Choose a reason for hiding this comment

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

this still needs adding, should be two lines.

@cinarizasyon
Copy link
Contributor Author

cinarizasyon commented Mar 1, 2022

@samuelcolvin Thanks for your reviews and valuable recommendations. I'll prepare a fix for bugs you found and add IsToday similar to IsNow.

dirty_equals/_datetime.py Show resolved Hide resolved
format_string: Optional[str] = None,
):

"""
Copy link
Owner

Choose a reason for hiding this comment

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

this still needs adding, should be two lines.

@samuelcolvin samuelcolvin merged commit 6e42e04 into samuelcolvin:main Mar 7, 2022
@samuelcolvin
Copy link
Owner

thanks so much.

@samuelcolvin samuelcolvin mentioned this pull request Mar 29, 2022
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants