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 lapse in vehicle_trips relation #20

Merged
merged 1 commit into from
Jun 17, 2021

Conversation

fonsecadeline
Copy link
Contributor

No description provided.

tsp_simple.cc Outdated Show resolved Hide resolved
tsp_simple.cc Outdated Show resolved Hide resolved
@fonsecadeline
Copy link
Contributor Author

@senhalil we initially had 'lapse' because it was initially used for mini/maximum day lapse (between visits). We could consider renaming it but either we have a different internal and external name for this relation.lapse, or we need to change fields for users (even though I am not sure much users use this field).

@senhalil
Copy link
Contributor

If that was the reason, we should definitely rename the internal variable (because it doesn't do what the name suggests) but also, when we have time, we can "deprecate" the lapse and replace it with duration so that all relations with a notion of duration (maximum_lapse, vehicle_group_duration, etc) can use it.

@senhalil
Copy link
Contributor

@fonsecadeline do you want to attack on the renaming here? I think its better if you create a ticket for the lapse -> duration renaming and deprecation, and we merge this without wait.. What do think?

@fonsecadeline
Copy link
Contributor Author

@senhalil I agree, like this we can consider to merge this (which is needed for multi-tour) as soon as I get back. Can you create the ticket ?

@senhalil
Copy link
Contributor

Don't know why do you want me to create the ticket, my message was clear its better if you create a ticket

@fonsecadeline
Copy link
Contributor Author

@senhalil but it is your idea.

@senhalil
Copy link
Contributor

senhalil commented Mar 1, 2021

I was the following up on the idea from your comment and I asked if you want to do the renaming here and gave my opinion that I think its better if you created a ticket. I think if a PR needs an action, it should be the creator of the PR who should take the action if they are able to.

@fonsecadeline
Copy link
Contributor Author

@senhalil there has been a misunderstood here. I felt you had a very clear idea of what we need to do that is why it would have make more sense for me if you wrote the ticket. From my point of view, if a PR pops an idea then the one with the suggestion could create the ticket.

Mais bref, this is not very important, and just a matter of point of vue :) the important here is that you approved PR so we can move on. I created general ticket and you can add your comments on it : https://gitlab.com/mapotempo/optimizer-api/-/issues/690.

@braktar do you approve this PR too ?

@fonsecadeline
Copy link
Contributor Author

fonsecadeline commented Jun 17, 2021

  • @fab-girard can you please change tag version to v1.4.1 after merging ? (v1.5 has been set)

@fab-girard fab-girard merged commit 54d4ebc into Mapotempo:dev Jun 17, 2021
@fonsecadeline fonsecadeline deleted the multi_tour_with_lapse branch June 17, 2021 14:22
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.

4 participants