Add transfers to route#515
Merged
Merged
Conversation
Closes #514
Closed
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR implements an improved path-finding algorithm in MetroMadrid by expanding the return type of getShortestPath to include detailed route information such as segments and transfers, and updating the MetroMadrid.vue component accordingly.
- The MetroMadrid.ts updates the getShortestPath method to return an object with path, distance, segments, and transfers.
- The MetroMadrid.vue component is updated to destructure the new return object from getShortestPath.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/lib/model/MetroMadrid.ts | Refactored getShortestPath to include transfers and segments. |
| src/components/MetroMadrid.vue | Adjusted destructuring assignment to match the updated return type. |
Comments suppressed due to low confidence (2)
src/components/MetroMadrid.vue:107
- [nitpick] Consider renaming 'stationDestiny' to 'stationDestination' for clarity and consistency with common naming conventions.
const stationDestiny: Station = this.metro.getStationById(this.selectedDestiny)
src/lib/model/MetroMadrid.ts:47
- [nitpick] The addition of transfers (and segments) in getShortestPath is a new behavior; consider adding tests to verify that transfers are correctly detected and recorded.
const transfers: Array<{ line: string; fromLine: string; toLine: string }> = []
Contributor
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces an enhanced route calculation feature to include transfers and segments, along with updated tests.
- Updated return type for getShortestPath to include the transfers and segments details.
- Adjusted test and Vue component code to reflect the new object structure returned by getShortestPath.
- Pending tests for transfers remain to be added.
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/model/MetroMadrid.spec.ts | Updated destructuring of getShortestPath output in tests. |
| src/lib/model/MetroMadrid.ts | Revised getShortestPath to include segments and transfers; added helper method. |
| src/components/MetroMadrid.vue | Adjusted destructuring to utilize the new getShortestPath return object. |
Files not reviewed (1)
- package.json: Language not supported
Comments suppressed due to low confidence (3)
test/model/MetroMadrid.spec.ts:40
- [nitpick] Consider renaming 'stationsAndDistance' to reflect its new structure containing path, distance, segments, and transfers for enhanced clarity.
let stationsAndDistance: any
src/components/MetroMadrid.vue:107
- [nitpick] Consider renaming 'stationDestiny' to 'stationDestination' for improved clarity and consistency.
const stationDestiny: Station = this.metro.getStationById(this.selectedDestiny)
test/model/MetroMadrid.spec.ts:47
- The test only verifies the path and distance; consider adding tests that validate the new segments and transfers properties.
it('should return the shortest path between two given stations', () => {
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds transfer information to the route calculation and display for Metro Madrid, and updates the associated components and tests.
- Introduces a new getShortestPath method that returns detailed path information including transfers
- Updates view components (StationList and StationItem) to incorporate transfer display
- Adjusts tests in MetroMadrid.spec.ts to match the new return type
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/model/MetroMadrid.spec.ts | Adjusting test destructuring for the updated getShortestPath return |
| src/lib/model/MetroMadrid.ts | Modifying getShortestPath to include transfer details |
| src/components/StationList.vue | Passing the transfer property to StationItem and updating component props |
| src/components/StationItem.vue | Adding markup to display transfer information |
| src/components/MetroMadrid.vue | Renaming variables and updating function calls for route retrieval |
Files not reviewed (1)
- package.json: Language not supported
Comments suppressed due to low confidence (1)
test/model/MetroMadrid.spec.ts:50
- Since getShortestPath now returns a path array of objects containing a 'station' property, update the mapping to: path.map(x => x.station.getId()).
const stations = path.map((x: Station) => x.getId())
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR introduces support for transfers in metro route calculations and updates related UI components while modifying test cases accordingly.
- Updated getShortestPath to return detailed route information including transfers
- Adjusted tests and UI components (StationList.vue, StationItem.vue, MetroMadrid.vue) to accommodate the new route structure
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/model/MetroMadrid.spec.ts | Updated test destructuring to adjust to the new getShortestPath return type |
| src/lib/model/MetroMadrid.ts | Revised getShortestPath to include transfer details and updated path construction |
| src/components/StationList.vue | Modified to pass additional transfer and currentLine properties to StationItem |
| src/components/StationItem.vue | Updated to render transfer information along with the current line info |
| src/components/MetroMadrid.vue | Adjusted usage of getShortestPath return values and updated shortestPath mapping |
Files not reviewed (1)
- package.json: Language not supported
Comments suppressed due to low confidence (2)
test/model/MetroMadrid.spec.ts:49
- Consider adding an assertion for the 'transfers' field (e.g., expect(transfers).to.deep.equal([])) to ensure the new transfers property is properly handled in tests.
const {path, distance} = stationsAndDistance
src/lib/model/MetroMadrid.ts:84
- [nitpick] The final station is represented with a self-referential segment. Consider setting 'segment' to null for the final station for clarity, since no travel transition occurs from that point.
path.push({ station: currentStation, segment: { line: currentLine!, from: currentStation, to: currentStation }, transfer: null })
Contributor
There was a problem hiding this comment.
Pull Request Overview
This pull request adds transfer support to metro route calculations, addressing issue #514.
- Refactored getShortestPath in MetroMadrid.ts to return transfer details along with path and distance.
- Updated tests to match the new return structure.
- Modified Vue components (StationList.vue, StationItem.vue, and MetroMadrid.vue) to display transfer information properly.
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/model/MetroMadrid.spec.ts | Updated test to destructure an object and map station IDs accordingly |
| src/lib/model/MetroMadrid.ts | Refactored getShortestPath to include transfers and update output structure |
| src/components/StationList.vue | Revised props and template to pass transfer and currentLine information |
| src/components/StationItem.vue | Modified props and template for conditional transfer display |
| src/components/MetroMadrid.vue | Updated variable naming and mapping of shortestPath to align with new structure |
Files not reviewed (1)
- package.json: Language not supported
Contributor
There was a problem hiding this comment.
Pull Request Overview
This pull request implements transfers into the metro route calculation by updating the shortest path algorithm and its consuming components while enhancing test coverage.
- Updated getShortestPath to return detailed route information including transfers.
- Modified tests and UI components (StationList, StationItem, MetroMadrid) to handle and display transfer data.
- Added a new LineBadge component for visualizing line information.
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/model/MetroMadrid.spec.ts | Updated test to reflect the new object structure for shortest path results. |
| src/lib/model/MetroMadrid.ts | Refactored getShortestPath to include transfer details and updated error handling. |
| src/components/StationList.vue | Updated to pass the new properties (transfer, currentLine) to the StationItem component. |
| src/components/StationItem.vue | Modified to support showing transfer information and applied a dynamic border style. |
| src/components/MetroMadrid.vue | Adjusted data binding and mapping logic to use the new route structure with transfers. |
| src/components/LineBadge.vue | Introduced a new component for displaying line badges with customized styling. |
Files not reviewed (1)
- package.json: Language not supported
Comments suppressed due to low confidence (2)
src/components/MetroMadrid.vue:107
- Consider renaming 'selectedDestiny' to 'selectedDestination' for clarity and consistency, and update its usage accordingly.
const stationDestination: Station = this.metro.getStationById(this.selectedDestiny)
src/lib/model/MetroMadrid.ts:20
- [nitpick] Consider defining a dedicated interface for the shortest path result that clearly documents the structure, enhancing type safety and maintainability.
public getShortestPath(
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds transfer information to the shortest path calculation and updates associated components and tests to handle the new route structure.
- Updated MetroMadrid model to return an object containing path, distance, and transfers.
- Revised test cases in MetroMadrid.spec.ts to reflect the new data structure.
- Updated StationList and StationItem components to display transfer details and adjust styling accordingly.
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/model/MetroMadrid.spec.ts | Updated test to use object destructuring for the new return type from getShortestPath. |
| src/lib/model/MetroMadrid.ts | Refactored getShortestPath to return detailed path objects and transfer information. |
| src/components/StationList.vue | Modified to pass proper properties (including transfer and currentLine) to StationItem. |
| src/components/StationItem.vue | Adjusted styling and added computed borderStyle dependent on transfer/currentLine values. |
| src/components/MetroMadrid.vue | Updated handling of the shortest path response and renamed a variable for clarity. |
| src/components/LineBadge.vue | Added a new component to display line information with proper styling. |
Files not reviewed (1)
- package.json: Language not supported
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds transfer logic to Metro Madrid route calculations and updates related components and tests accordingly. Key changes include:
- Revising the getShortestPath method in MetroMadrid.ts to include detailed path segments and transfer information.
- Updating tests to reflect the new data structure and behavior.
- Modifying StationList.vue, StationItem.vue, and MetroMadrid.vue to support and display the new transfer data.
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/model/Station.spec.ts | Updated tests with new edge cases and modernized test functions. |
| test/model/MetroMadrid.spec.ts | Adapted tests for the updated getShortestPath return type. |
| test/model/Line.spec.ts | Refactored tests utilizing direct property access and setters. |
| src/lib/model/MetroMadrid.ts | Revised getShortestPath to include segments, transfers, and error checks. |
| src/components/StationList.vue | Updated prop passing to match the new transfer and segment structure. |
| src/components/StationItem.vue | Adjusted UI rendering and computed border style to reflect transfer data. |
| src/components/MetroMadrid.vue | Modified mapping logic to work with the updated path structure. |
| src/components/LineBadge.vue | Introduced a dedicated component for rendering line badges. |
Files not reviewed (1)
- package.json: Language not supported
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #514