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

Creating a single Sign protobuf object that can be used by both directions.proto and trip.proto. #3146

Merged
merged 6 commits into from
Jun 16, 2021

Conversation

ktatterso
Copy link
Contributor

@ktatterso ktatterso commented Jun 14, 2021

Both trip.proto and directions.proto have Sign and SignElement objects that are very similar. I've unified these into sign.proto. No new tests are needed as existing tests should be sufficient for this refactor.

Also, on Mac, -Werror is too strong for all of mjolnir, the Boost header format.h fails to compile with -Werror. So I addressed that issue as well.

Issue

A simple refactor. Will help in the future if we have functions/methods that extract Sign information. (I'm planning to do this exact thing in a near PR.)

Tasklist

  • Add tests
  • Add #fixes with the issue number that this PR addresses
  • Update the docs with any new request parameters or changes to behavior described
  • Update the changelog
  • If you made changes to the lua files, update the taginfo too.

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

Created a single sign.proto that could be shared by trip.proto and directions.proto, since they are nearly the same.
Copy link
Member

@dgearhart dgearhart left a comment

Choose a reason for hiding this comment

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

The build is failing
It is always good practice to run RAD before and after in a addition to the unit and gurka

@ktatterso
Copy link
Contributor Author

ktatterso commented Jun 15, 2021

The build is failing
It is always good practice to run RAD before and after in a addition to the unit and gurka

@dgearhart
Its weird, I'm on osx and my build is fine, but circleci's osx build is failing (on this thing Mandeep mentioned yesterday... and the circleci build-release is failing on a Werror thing that builds fine for me (on osx) but not on Linux. Of course I always run unit/gurka tests, they are clean. I didn't run RAD because it was just a refactor, but I can.

@dgearhart dgearhart dismissed their stale review June 15, 2021 20:55

Defer to others

@@ -57,6 +49,14 @@ set(sources
set (sources_with_warnings
bssbuilder.cc
dataquality.cc
elevationbuilder.cc
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are the files not building on OSX with -Werror.

gknisely
gknisely previously approved these changes Jun 16, 2021
@ktatterso ktatterso merged commit 118ffe7 into master Jun 16, 2021
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.

None yet

3 participants