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

Fix hour.at('MM:SS') parsing bug #290

Merged
merged 2 commits into from
Jan 13, 2021
Merged

Conversation

eladbi
Copy link
Contributor

@eladbi eladbi commented Mar 7, 2019

Hi,

This fix/feature was created after viewing issue #286:
#286

It's the first time I'm trying to change something here, so please excuse me if I didn't understand to wanted behaviour, I will appreciate any feedback that will be given to me :)

My first intension was to change the code so hour.at() could only get 'MM:SS' format instead of the current ':MM' format, because I could understand why @tangb4c think that this is the expected format.
I also saw in the code that there is no support for 'MM:SS' format but it doesn't raise an exception when the user use hour.at('HH:MM') format.

I didn't change it like I wanted, because this will cause unwanted behaviour for people who currently running the code with ':MM' format, So i decided that i will keep the old behaviour as it is, and add the support for 'MM:SS' format

If the new fix/feature is not wanted for any reason, I will strongly advise to raise an error when a user try to run hour.at(''MM:SS').

Thanks
Elad

@eladbi eladbi changed the title Allow 'MM:SS' format for hour.at while keeping the current format ':M… Allow 'MM:SS' format for hour.at() - issue #286 Mar 7, 2019
@coveralls
Copy link

coveralls commented Mar 7, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 18a4b3a on eladbi:issue286 into b3e75d2 on dbader:master.

@eladbi
Copy link
Contributor Author

eladbi commented Mar 7, 2019

Hi @dbader,

I will appreciate if you could help me here,
When running the test (local environment) all 24 pass, but i didn't saw 26 tests, but when looking at the
logs (attached) I saw that schedule/init.py failed and also test_schedule.py failed, but i'm not sure how can I test it local.
![Uploading Screen Shot 2019-03-07 at 16.01.17.png…]

@SijmenHuizenga
Copy link
Collaborator

Hi @eladbi, thanks a bunch for submitting this pr!

I ran some tests to confirm the behaviour. These tests ran at 01:15:55.

Before the change:

print(schedule.every().hour.at(":58").do(job).next_run)
# 01:58:00

print(schedule.every().hour.at("24:58").do(job).next_run)
# 01:58:00 (24 is being ignored)

After the change:

print(schedule.every().hour.at(":58").do(job).next_run)
#  01:58:00

print(schedule.every().hour.at("24:58").do(job).next_run)
# 01:24:58    <--- breaking change

The latter is definitely more desirable.

However, this is a breaking change and I'm not sure what the impact would be. How many people are using .hours.at() in a way that it will change behaviour? Theoretically the maximum functional difference between before and after would be around 30 minutes (that is if one used 00:30), some won't notice the difference but some places 30 minutes is marjor. And that doesn't account for the use-cases where the .at() is used dynamically based on the current time, it might even break more software.

This library has been upward compatible for many years and I would like to keep it that way.

So what now? I don't know yet.

@SijmenHuizenga SijmenHuizenga changed the title Allow 'MM:SS' format for hour.at() - issue #286 Fix hour.at('MM:SS') parsing bug Jan 8, 2021
@SijmenHuizenga SijmenHuizenga added this to the 1.0.0 milestone Jan 9, 2021
@SijmenHuizenga
Copy link
Collaborator

After some thinking I have come to the conclusion that we can merge this and release it as part of the next major release (v1.0.0). See #412

@SijmenHuizenga
Copy link
Collaborator

Rebased on master to resolve merge conflicts

@SijmenHuizenga
Copy link
Collaborator

No idea why the code coverage keeps failing... Will have to investigate further some other time.

@SijmenHuizenga
Copy link
Collaborator

Solved the failing build on master. Rebased these commits and it is green 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants