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 https://github.com/ipeaGIT/r5r/issues/327 #354

Merged
merged 1 commit into from
Nov 20, 2023

Conversation

luyuliu
Copy link
Contributor

@luyuliu luyuliu commented Oct 20, 2023

A simple fix.

A simple fix.
@luyuliu
Copy link
Contributor Author

luyuliu commented Oct 20, 2023

I just tested this on detailed_itineraries(). I do not know if other functions would also use tryRunProcess to output csv though.

@rafapereirabr
Copy link
Member

Hi @luyuliu . Thanks so much for opening this PR. I believe other functions in r5r also use tryRunProcess, so it would be necessary to check to what extent the code you edited would also affect other functions. Perhaps this is something @mvpsaraiva could have a look at ?

@luyuliu
Copy link
Contributor Author

luyuliu commented Nov 2, 2023

I did a quick test on other function (travel_time_matrix) with the parameter output_dir. It seems to be working, but the file name would be like from_xxx_xxx, with the second id being the same as the origin id. It doesn't seem to break the function, so it can be a temporary solution.

The best practice would be write function specific condition clause inside the tryRunProcess function based on the function that calls it. Or, it would be even better to have a dedicated output function to output csv.

@mvpsaraiva
Copy link
Collaborator

thanks @luyuliu. I'll merge this PR and fix the file names for the travel time matrices functions following your suggestion.

@mvpsaraiva mvpsaraiva merged commit a0b956d into ipeaGIT:master Nov 20, 2023
@rafapereirabr
Copy link
Member

@luyuliu , thanks again for fixing this issue. We'll be adding you as a contributor to r5r if that's ok with you

@luyuliu
Copy link
Contributor Author

luyuliu commented Nov 23, 2023

Thanks. I’m glad to be listed as a contributor.

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.

None yet

3 participants