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 feed id column to path results #927

Closed
wants to merge 25 commits into from
Closed

Conversation

ansoncfit
Copy link
Member

@ansoncfit ansoncfit commented Jan 25, 2024

This PR addresses #920, adding a column for feedIds to CSV path detail results. It also adds a "group" column to these results, reserved for future use. As noted in the discussion below and in #920, we are deriving the feedId from the boarding stop for convenience; this should generally correspond with the feedId of the route, but it may not (if, for example, a route has been subject to a reroute modification sending it to a stop from a different feed).

Because of some clumsy rebasing, the diff here looks larger than it is. See instead dev...results-with-feedids

abyrd and others added 10 commits December 20, 2023 14:04
Many constants (as indicated by capitalization) were not static.
One field always set in the constructor was not final.
The list of resultAssemblers WAS static. This may be a remnant of pre-
component design. This was not problematic because we never construct
more than one Broker, but it should be possible to do (no global state).
This change adds a new helper method to pass a `fields` object to `MongoMap#findByIdIfPermitted` and utilizes that to exclude modifications while looking up a regional analysis to generate a `scenarioJsonUrl`.

This will prevent errors from occurring if custom modifications are used in a worker version that do not exist in the server.
Final and static field modifiers in Broker
@abyrd
Copy link
Member

abyrd commented Feb 2, 2024

As recently discussed: the number of fields will remain fixed. The feed ID field will always be present, but will be empty unless it is intentionally switched on. That is, it won't be automatically enabled even when there are multiple feeds in a bundle. It will be enabled using the "flags" approach demonstrated in #930.

I'm starting to wonder though whether this merits a structured CsvOptions in the request:

{
  csvOptions: {
    stopRepresentation: ID_AND_NAME,
    routeRepresentation: NAME_ONLY,
    includeFeedIds: true
  }
}

While we're already considering increasing the hard-wired number of columns, I'd like to suggest adding another empty-by-default field called "label" or "group", which would be used to facilitate filtering and grouping CSV output rows in specialized use cases (such as select-link).

@abyrd
Copy link
Member

abyrd commented Feb 29, 2024

A few comments on the current implementation discussed during the call:

  • EntityRepresentation is ignored (we only check if it's null) - we always show the ID (not name) for now
  • FeedId is that of the boarding stop, which is usually the same as the feedId of the alighting stop and the route, but might be mismatched if modifications have mixed routes and stops from different feeds
  • We are grabbing the feed ID from the stop rather than the route (which might seem like a better representative of the leg) because stops happen to have a readily available feed ID

@ansoncfit ansoncfit requested a review from abyrd March 5, 2024 21:40
@ansoncfit ansoncfit enabled auto-merge March 8, 2024 13:55
@ansoncfit ansoncfit dismissed abyrd’s stale review March 20, 2024 16:48

The merge-base changed after approval.

abyrd
abyrd previously approved these changes Mar 20, 2024
Copy link
Member

@abyrd abyrd left a comment

Choose a reason for hiding this comment

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

Looks good (when looking at the alternate diff view you linked to). I wish I understood better why the default diff ends up looking this way due to a rebase, but hopefully the merge will work correctly.

abyrd
abyrd previously approved these changes Mar 20, 2024
@ansoncfit ansoncfit dismissed abyrd’s stale review March 20, 2024 16:54

The merge-base changed after approval.

@abyrd
Copy link
Member

abyrd commented Mar 20, 2024

Something strange is going on with the review process:
image
Even if I approve it Github says a review is required. When I do a git diff from this branch to dev locally, I see the expected set of changes which is different than the changes visible here.

@abyrd
Copy link
Member

abyrd commented Mar 21, 2024

Superseded by #936 to overcome apparent bad state on the Github side.

@abyrd abyrd closed this Mar 21, 2024
auto-merge was automatically disabled March 21, 2024 01:57

Pull request was closed

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.

3 participants