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 shift() method #27

Merged
merged 11 commits into from
Jun 6, 2020
Merged

Add shift() method #27

merged 11 commits into from
Jun 6, 2020

Conversation

alex-verve
Copy link
Contributor

refs #20

@alex-verve alex-verve marked this pull request as ready for review June 5, 2020 17:19
Copy link
Owner

@adamchainz adamchainz left a comment

Choose a reason for hiding this comment

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

Thanks !

src/time_machine.py Outdated Show resolved Hide resolved
src/time_machine.py Outdated Show resolved Hide resolved
@adamchainz
Copy link
Owner

So I'm thinking this through a bit. I am not sure if it's wise to pursue 100% the same API as freezegun here because it's a bit confusing.

The name tick has two distinct meanings. The tick argument to travel() 'makes sense' to me - it "keeps the clock ticking." The tick() method - well it offsets the current time, even in reverse.

I'm also not sold on the default being 1 second. When is that useful case? Wouldn't it better to be explicit, for the sake of writing '1' in tests to make it clear how much time is moving by?

I'm gonna mull this over for a little bit, it's especially a question as to whether I want this library to have the exact same API as freezegun, or just to support the same use cases. There's some support for copying the core technique over to freezegun so I might be free-er to use a different API here.

@alex-verve
Copy link
Contributor Author

The tick() method - well it offsets the current time, even in reverse.

Totally agree, it is confusing to have two different things called tick.
I think that idea of having different API is wise in that case (as long as time-machine has covered all use cases).

What do you think of these?

def test_coordinates_add_timedelta():
    with time_machine.travel(EPOCH, tick=False) as coordinates:
        coordinates += dt.timedelta(seconds=1)

        assert time.time() == EPOCH + 1


def test_coordinates_set_exact_time():
    destination_dt = dt.datetime(2000, 1, 1)

    with time_machine.travel(EPOCH, tick=False) as traveller:
        traveller.move_to(destination_dt)

        # or maybe even? :)
        traveller >> destination_dt

        assert time.time() == destination_dt.timestamp()

@adamchainz
Copy link
Owner

I don't like operator overloading, it is a bit implicit. Perhaps shift()? Or move_offset() ?

@alex-verve
Copy link
Contributor Author

I like both shift() and move_offset() 😃
Did you consider having one method for both things: move(offset=..)/move(to=..)? (not sure if it's good idea tho).
Maybe we need to hear opinion from thee wider public?

README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
Co-authored-by: alex-verve <alex.subbotin@pollen.co>
@adamchainz
Copy link
Owner

I've gone with shift() as you've seen. I think it's a good name, better than tick(). If anyone finds a problem with it, we can always add an alias.

@adamchainz adamchainz changed the title Tick method Add shift() method Jun 6, 2020
README.rst Outdated Show resolved Hide resolved
Co-authored-by: alex-verve <alex.subbotin@pollen.co>
README.rst Outdated Show resolved Hide resolved
Co-authored-by: alex-verve <alex.subbotin@pollen.co>
@adamchainz adamchainz merged commit af5e5b3 into adamchainz:master Jun 6, 2020
@alex-verve alex-verve deleted the tick_method branch June 6, 2020 18:00
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