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 a checkbox to path edit form to allow ignore #784

Merged

Conversation

kaligrafy
Copy link
Collaborator

default dwell times for associated nodes, and add help to default dwell time

@kaligrafy
Copy link
Collaborator Author

Please test in the ui as well. I have problems with my localhost config and I can't test json2Capnp and osrm right now, so can't test the paths themselves.

@tahini
Copy link
Collaborator

tahini commented Nov 29, 2023

The test failure looks legit

@tahini
Copy link
Collaborator

tahini commented Nov 29, 2023

The ignore checkbox does not seem to have any effect...

My nodes have a dwell time of 20 seconds, I put 10, and whether I ignore or not, the stats are the same. If I change the dwell time of the nodes to 10, then the stats do decrease, so it's really the ignore checkbox not working.

@tahini
Copy link
Collaborator

tahini commented Nov 29, 2023

But your unit test failing may be a sign of something wrong, maybe if you fix the test, it will all be fixed!

@kaligrafy kaligrafy force-pushed the addIgnoreNodesDefaultDwellTimeInPath branch from f6e82a5 to 952a6e0 Compare November 29, 2023 17:07
Copy link
Collaborator

@tahini tahini left a comment

Choose a reason for hiding this comment

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

Typo on the data field name, but when fixed, it works well

packages/transition-common/src/services/path/Path.ts Outdated Show resolved Hide resolved
@kaligrafy kaligrafy force-pushed the addIgnoreNodesDefaultDwellTimeInPath branch from 952a6e0 to 57dfb8c Compare November 29, 2023 18:02
@kaligrafy kaligrafy requested a review from tahini November 29, 2023 18:04
@kaligrafy kaligrafy force-pushed the addIgnoreNodesDefaultDwellTimeInPath branch from 57dfb8c to 19c60b0 Compare November 29, 2023 18:12
@kaligrafy kaligrafy requested a review from tahini November 29, 2023 18:12
Copy link
Collaborator

@tahini tahini left a comment

Choose a reason for hiding this comment

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

You probably should do a find & replace on the whole code to see if any other typos are lying around...

@kaligrafy kaligrafy force-pushed the addIgnoreNodesDefaultDwellTimeInPath branch from 19c60b0 to 14602b0 Compare November 29, 2023 18:35
default dwell times for associated nodes, and add help to default dwell time
@kaligrafy kaligrafy force-pushed the addIgnoreNodesDefaultDwellTimeInPath branch from 14602b0 to ae2fbc6 Compare November 29, 2023 18:36
@kaligrafy kaligrafy requested a review from tahini November 29, 2023 18:36
@tahini tahini merged commit dd27137 into chairemobilite:main Nov 29, 2023
4 checks passed
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