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

Unset DELETE method on Windows #161

Merged
merged 6 commits into from
Jan 27, 2021
Merged

Unset DELETE method on Windows #161

merged 6 commits into from
Jan 27, 2021

Conversation

j-rivero
Copy link
Contributor

Delete is a reserved word on Windows. This PR runs the workaround of unset it so the ResultType class can compile. To real fix the problem I think that we need to plan a migration from using the reserved word to another (REMOVE or something like that)

@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label Jan 14, 2021
@chapulina chapulina added the Windows Windows support label Jan 14, 2021
@chapulina
Copy link
Contributor

How about renaming it already? Maybe DEL? We could have both DEL and DELETE on v6 and remove DELETE on v7.

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

DCO needs to be signed and codecheck fixed. Renaming DELETE doesn't need to be addressed on this PR.

@@ -29,6 +29,8 @@
// std::string
#pragma warning(push)
#pragma warning(disable: 4251)
// TODO: rename the DELETE method which is a reserved word in Windows
Copy link
Contributor

Choose a reason for hiding this comment

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

codecheck is failing:

  /github/workspace/include/ignition/fuel_tools/RestClient.hh:32:  Missing username in TODO; it should look like "// TODO(my_username): Stuff."  [readability/todo] [2]
  /github/workspace/include/ignition/fuel_tools/Result.hh:31:  Missing username in TODO; it should look like "// TODO(my_username): Stuff."  [readability/todo] [2]

chapulina and others added 3 commits January 26, 2021 18:41
Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
…ls into win_delete

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
@codecov
Copy link

codecov bot commented Jan 26, 2021

Codecov Report

Merging #161 (31e03f1) into main (964db6f) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #161   +/-   ##
=======================================
  Coverage   77.85%   77.85%           
=======================================
  Files          19       19           
  Lines        2606     2606           
=======================================
  Hits         2029     2029           
  Misses        577      577           
Impacted Files Coverage Δ
include/ignition/fuel_tools/RestClient.hh 100.00% <ø> (ø)
include/ignition/fuel_tools/Result.hh 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 964db6f...31e03f1. Read the comment docs.

@chapulina chapulina merged commit 0f0b1ee into main Jan 27, 2021
@chapulina chapulina deleted the win_delete branch January 27, 2021 03:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏢 edifice Ignition Edifice Windows Windows support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants