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

feat: Validate that combination of route names and type are unique wi… #839

Merged
merged 3 commits into from
Mar 30, 2021

Conversation

aababilov
Copy link
Collaborator

…thin an agency

This logic corresponds to real-world use cases validated at Google for
all its production feeds.

Previous validation produced too many false positives because it ignored
route type and validated uniqueness of short and long names separately.

…thin an agency

This logic corresponds to real-world use cases validated at Google for
all its production feeds.

Previous validation produced too many false positives because it ignored
route type and validated uniqueness of short and long names separately.
Copy link
Contributor

@lionel-nj lionel-nj left a comment

Choose a reason for hiding this comment

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

Thanks @aababilov LGTM

Copy link
Contributor

@lionel-nj lionel-nj left a comment

Choose a reason for hiding this comment

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

Just noticed that README.md should be updated to reflect the changes.

@lionel-nj lionel-nj dismissed their stale review March 30, 2021 16:30

trying to dismiss the first review

Copy link
Contributor

@lionel-nj lionel-nj left a comment

Choose a reason for hiding this comment

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

LGTM! Just one thing: README.md should be updated to reflect the changes.

Thanks @aababilov

@aababilov
Copy link
Collaborator Author

Done!

@lionel-nj
Copy link
Contributor

Thanks @aababilov ! :)

@aababilov
Copy link
Collaborator Author

Thanks for review, @lionel-nj !

Copy link
Contributor

@lionel-nj lionel-nj left a comment

Choose a reason for hiding this comment

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

Just one typo and then we're good to go :)

RULES.md Outdated Show resolved Hide resolved
Copy link
Contributor

@lionel-nj lionel-nj left a comment

Choose a reason for hiding this comment

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

👍🏾 ✅ 🚀

@lionel-nj lionel-nj merged commit 647d78c into MobilityData:master Mar 30, 2021
@aababilov aababilov deleted the duplicate-route-name branch April 20, 2021 02:24
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.

2 participants