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

Refactor/use format #1084

Merged
merged 5 commits into from
May 23, 2024
Merged

Conversation

SebMilardo
Copy link
Contributor

@SebMilardo SebMilardo commented Apr 11, 2024

Issue

Fixes #1081

Tasks

  • Use std::format instead of concatenating pieces manually
  • review

Copy link
Collaborator

@jcoupey jcoupey left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Looks great overall, the error messages are much more readable now.

Sonarcloud is complaining about not using raw string literals to avoid escaping, but I don't know how this plays along with std::format?

src/structures/vroom/input/input.cpp Outdated Show resolved Hide resolved
@SebMilardo
Copy link
Contributor Author

Thanks for the PR! Looks great overall, the error messages are much more readable now.

Sonarcloud is complaining about not using raw string literals to avoid escaping, but I don't know how this plays along with std::format?

It looks like I have to rewrite them as:

R"({{"lon":{},"lat":{},"type":"break"}},)"

and

R"({{"lon":{},"lat":{}}},)"

Let me push a commit...

@jcoupey
Copy link
Collaborator

jcoupey commented Apr 11, 2024

Thanks for the changes, looks all good to me.

The only weird thing is we'll have to wait to merge since our compiler versions in CI will error on std::format currently. Thus there is no point in enabling the CI checks right now either since they're bound to fail. Let's leave this open for now and revisit once #1080 lands.

@jcoupey
Copy link
Collaborator

jcoupey commented May 22, 2024

We should now be able to let this run through the CI checks after upgrading compiler versions in #1113.

@SebMilardo could you pull the latest master on your fork and git merge --no-ff master into your PR branch? I did just that but it looks like I'm not allowed to push to your fork.

@SebMilardo
Copy link
Contributor Author

It should be ok now

@jcoupey jcoupey added this to the v1.15.0 milestone May 22, 2024
Copy link
Collaborator

@jcoupey jcoupey left a comment

Choose a reason for hiding this comment

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

Thanks for the update, CI builds are OK now! \o/

src/routing/valhalla_wrapper.cpp Show resolved Hide resolved
@jcoupey jcoupey merged commit 9e19715 into VROOM-Project:master May 23, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use std::format
2 participants