-
Notifications
You must be signed in to change notification settings - Fork 1
Merge route networks #16
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
Conversation
binh-dam-ibigroup
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
route_networks.txt is exported incorrectly with the headers and content mixed up.
| /** | ||
| * Create a CSV row from original fields plus grouped (stop areas or route network) ids. | ||
| */ | ||
| protected static String createRow(CsvReader stopsReader, String ids, String[] csvFields) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename stopsReader to csvReader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in: 82684ac
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename stopsReader back to csvReader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in: a65a86a
| "%s,%s%n", | ||
| String.join(",", CSV_FIELDS), | ||
| ROUTE_NETWORK_IDS_FIELD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Is there is a better way to concat one item to an array, or is a preset array or a list better for this purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about this? Not sure if it is any better: 82684ac
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, no, can you revert?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted: d63eaca
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@binh-dam-ibigroup I've reverted these two changes. Is this good to go now?
|
|
||
| private static final String CSV_HEADER_FOR_EXPORT = String.format( | ||
| "%s", | ||
| String.join(",", CSV_FIELDS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove String.format.
binh-dam-ibigroup
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest changes seem to work. Consider including the refactors from #17.
refactor(Entity): Extract logic for printing col after existing ones.
Checklist
devbefore they can be merged tomaster)Description
Merge route networks into routes.