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

Flex spec v2 #7

Merged
merged 28 commits into from
Oct 22, 2024
Merged

Conversation

br648
Copy link

@br648 br648 commented Jul 11, 2024

Checklist

  • Appropriate branch selected (all PRs must first be merged to dev before they can be merged to master)
  • Any modified or new methods or classes have helpful JavaDoc and code is thoroughly commented
  • The description lists all applicable issues this PR seeks to resolve
  • The description lists any configuration setting(s) that differ from the default settings
  • All tests and CI builds passing

Description

Changes to migrate the dev-flex branch from flex version 1 to version 2.

Copy link

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

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

First round of comments for now with some cleanup suggestions, much more to come!

src/main/java/com/conveyal/gtfs/PatternBuilder.java Outdated Show resolved Hide resolved
src/main/java/com/conveyal/gtfs/PatternBuilder.java Outdated Show resolved Hide resolved
src/main/java/com/conveyal/gtfs/PatternBuilder.java Outdated Show resolved Hide resolved
src/main/java/com/conveyal/gtfs/PatternFinder.java Outdated Show resolved Hide resolved
src/main/java/com/conveyal/gtfs/PatternFinder.java Outdated Show resolved Hide resolved
Copy link

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

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

Next round of comments and more to come!

Copy link

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

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

Other round of comments. Next is for me to try running this thing.

Copy link

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

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

@br648 I am going to hand this over so you can make the changes per all previous comments.

@binh-dam-ibigroup binh-dam-ibigroup removed their assignment Aug 16, 2024
br648 and others added 9 commits August 20, 2024 14:57
Co-authored-by: Binh Dam <56846598+binh-dam-ibigroup@users.noreply.github.com>
Co-authored-by: Binh Dam <56846598+binh-dam-ibigroup@users.noreply.github.com>
Co-authored-by: Binh Dam <56846598+binh-dam-ibigroup@users.noreply.github.com>
Co-authored-by: Binh Dam <56846598+binh-dam-ibigroup@users.noreply.github.com>
Co-authored-by: Binh Dam <56846598+binh-dam-ibigroup@users.noreply.github.com>
Co-authored-by: Binh Dam <56846598+binh-dam-ibigroup@users.noreply.github.com>
…tor.java

Co-authored-by: Binh Dam <56846598+binh-dam-ibigroup@users.noreply.github.com>
@br648 br648 assigned binh-dam-ibigroup and unassigned br648 Aug 30, 2024
Copy link

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

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

So far, there is a big typo in the validation messages that needs to be fixed.

Copy link

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

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

A few more typos and light refactorings then I will approve. I don't feel too strongly about extracting the JSON data out of the code file. The basic functionality seems to work with corresponding datatools server and UI branches.

src/main/java/com/conveyal/gtfs/model/Location.java Outdated Show resolved Hide resolved
@br648 br648 assigned binh-dam-ibigroup and unassigned br648 Oct 16, 2024
Copy link

@JymDyerIBI JymDyerIBI left a comment

Choose a reason for hiding this comment

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

👍 Buncha changes, all look good.

Copy link

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

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

Please address the two minor fixes before merging.

src/main/java/com/conveyal/gtfs/util/GeoJsonUtil.java Outdated Show resolved Hide resolved
@JymDyerIBI
Copy link

Death, where is thy Sting? Last edit looks good.

@br648 br648 merged commit a0b5eef into dev-flex Oct 22, 2024
2 checks passed
Copy link

@JymDyerIBI JymDyerIBI left a comment

Choose a reason for hiding this comment

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

Change looks good.

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