search

florianv / business

:date: DateTime calculations in business hours
http://florianv.github.io/business
MIT License
361 stars 25 forks source link

Return the actual opening time for the closest opening time in a day. #21

Closed dragosprotung closed 8 years ago

florianv commented 8 years ago

This change is not backward compatible.

I think you should add a third parameter to the closest method to specify if we must return the closing or opening time.

dragosprotung commented 8 years ago

It's true, it's not backward compatible, but I do not think adding another parameter to the function is good design.

I created this PR more to start a conversation. I think the current behavior is wrong or the naming totaly misleading. If the function is called getClosestOpeningTimeBefore shouldn't it return the opening time ? Why does it return the closing time or the current time if the time is in an interval ?

I do not know what your plans are for the library, but I like to propose to update the library with new methods, that cover current behavior as well, and release the library as version 2. Of course I am willing to create a new PR for this.

florianv commented 8 years ago

I think the current behavior is wrong or the naming totaly misleading. If the function is called getClosestOpeningTimeBefore shouldn't it return the opening time ? Why does it return the closing time or the current time if the time is in an interval ?

I agree the name is maybe misleading, but the function returns the closest "opening" time which is the closing time of the last interval. Maybe it could have been named getClosestBusinessTimeBefore.

I do not know what your plans are for the library, but I like to propose to update the library with new methods, that cover current behavior as well, and release the library as version 2. Of course I am willing to create a new PR for this.

There are no plans as it fits my initial use case, but if you wish to add more features they are welcomed :)

dragosprotung commented 8 years ago

Definitely there is confusion in the naming. But how I see it, there should be methods that return

I know this will break BC. Are willing to accept a PR with this big changes and release it as version 2 ? I am willing to work on this big change. What is the roadmap for this lib ?

florianv commented 8 years ago

I'm closing this PR as it's not mergeable. Please feel free to create an issue with the suggested changes.

Changing two methods don't deserve to release a new version. What you suggest can be achieved by adding a new argument / option to each method

dragosprotung commented 8 years ago

I understand. I ended up writing a new library to achieve the desired behaviour. You can have a look here

It is inspired by your library, is missing a few features, but I feel is more extendible and a bit more consistent.

florianv commented 8 years ago

It is inspired by your library

It's not inspired by my library. You are copying the concept, using the same class names, some parts of the code, the same method names under your namespace & copyright.

I'm fine with it if my name clearly appears on the project copyright.

dragosprotung commented 8 years ago

I added link to your repo.

florianv commented 8 years ago

@dragosprotung I don't need a link to my repo. Legally my copyright has to remain in all copied files headers and in the project LICENSE

phproberto commented 8 years ago

So you duplicate all the code and "steal" it because you need to change its behavior?

You just need to write replacements for the classes whose functionality you want to change.