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

Add DictionaryTrait #573

Merged
merged 12 commits into from
Sep 28, 2023
Merged

Add DictionaryTrait #573

merged 12 commits into from
Sep 28, 2023

Conversation

RiscadoA
Copy link
Member

@RiscadoA RiscadoA commented Sep 22, 2023

Closes #484, #541 and #542.

Description

Adds the DictionaryTrait and defines reflection for both std::map<K, V> and std::unordered_map<K, V>.

Checklist

  • Self-review changes.
  • Evaluate impact on the documentation.
  • Ensure test coverage.
  • Write new samples.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 22, 2023

PR Preview Action v1.4.4
🚀 Deployed preview to https://GameDevTecnico.github.io/cubos/pr-preview/pr-573/
on branch gh-pages at 2023-09-28 10:01 UTC

@codecov
Copy link

codecov bot commented Sep 22, 2023

Codecov Report

Attention: 22 lines in your changes are missing coverage. Please review.

Comparison is base (272177a) 31.80% compared to head (20be5a0) 33.92%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #573      +/-   ##
==========================================
+ Coverage   31.80%   33.92%   +2.11%     
==========================================
  Files          92       95       +3     
  Lines        6357     6594     +237     
==========================================
+ Hits         2022     2237     +215     
- Misses       4335     4357      +22     
Files Coverage Δ
...ore/include/cubos/core/reflection/external/map.hpp 90.00% <90.00%> (ø)
...e/cubos/core/reflection/external/unordered_map.hpp 90.00% <90.00%> (ø)
...re/src/cubos/core/reflection/traits/dictionary.cpp 91.24% <91.24%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@RiscadoA RiscadoA force-pushed the 484-add-dictionarytrait branch from c6806f3 to a021941 Compare September 26, 2023 14:08
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ clang-tidy found issue(s) with the introduced code (1/7)

core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ clang-tidy found issue(s) with the introduced code (2/7)

core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/traits/dictionary.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/traits/dictionary.hpp Outdated Show resolved Hide resolved
core/tests/reflection/traits/dictionary.hpp Show resolved Hide resolved
core/include/cubos/core/reflection/traits/dictionary.hpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ clang-tidy found issue(s) with the introduced code (3/7)

core/include/cubos/core/reflection/traits/dictionary.hpp Outdated Show resolved Hide resolved
core/src/cubos/core/reflection/traits/dictionary.cpp Outdated Show resolved Hide resolved
core/src/cubos/core/reflection/traits/dictionary.cpp Outdated Show resolved Hide resolved
core/src/cubos/core/reflection/traits/dictionary.cpp Outdated Show resolved Hide resolved
core/src/cubos/core/reflection/traits/dictionary.cpp Outdated Show resolved Hide resolved
core/src/cubos/core/reflection/traits/dictionary.cpp Outdated Show resolved Hide resolved
core/src/cubos/core/reflection/traits/dictionary.cpp Outdated Show resolved Hide resolved
core/src/cubos/core/reflection/traits/dictionary.cpp Outdated Show resolved Hide resolved
core/src/cubos/core/reflection/traits/dictionary.cpp Outdated Show resolved Hide resolved
core/src/cubos/core/reflection/traits/dictionary.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ clang-tidy found issue(s) with the introduced code (4/7)

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ clang-tidy found issue(s) with the introduced code (5/7)

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ clang-tidy found issue(s) with the introduced code (6/7)

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ clang-tidy found issue(s) with the introduced code (7/7)

core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ clang-tidy found issue(s) with the introduced code (1/2)

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ clang-tidy found issue(s) with the introduced code (2/2)

core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/external/map.hpp Outdated Show resolved Hide resolved
@RiscadoA RiscadoA force-pushed the 484-add-dictionarytrait branch from 7283c70 to 4305cf1 Compare September 26, 2023 14:31
@RiscadoA RiscadoA marked this pull request as ready for review September 26, 2023 14:39
@RiscadoA
Copy link
Member Author

The coverage isn't so great on this PR, but I'm not sure if we should worry too much about it, at least for now.
What do you think? @luishfonseca

Copy link
Contributor

@luishfonseca luishfonseca left a comment

Choose a reason for hiding this comment

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

Should we add .view(...)? It would allow the following syntax

for (auto [k, v] : dictionaryTrait.view(myDict))
{
...
}

@luishfonseca
Copy link
Contributor

The coverage isn't so great on this PR, but I'm not sure if we should worry too much about it, at least for now. What do you think? @luishfonseca

It's fine, it will get covered as it gets used

@RiscadoA RiscadoA force-pushed the 484-add-dictionarytrait branch from ba097ff to b81b3b2 Compare September 28, 2023 07:57
@github-actions github-actions bot dismissed stale reviews from themself September 28, 2023 07:58

No clang-tidy warnings found so I assume my comments were addressed

@RiscadoA RiscadoA force-pushed the 484-add-dictionarytrait branch from ea5f292 to 00d4a24 Compare September 28, 2023 09:59
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ clang-tidy found issue(s) with the introduced code (1/1)

core/include/cubos/core/reflection/traits/dictionary.hpp Outdated Show resolved Hide resolved
core/include/cubos/core/reflection/traits/dictionary.hpp Outdated Show resolved Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@RiscadoA RiscadoA enabled auto-merge (rebase) September 28, 2023 10:02
@github-actions github-actions bot dismissed their stale review September 28, 2023 10:03

No clang-tidy warnings found so I assume my comments were addressed

@RiscadoA RiscadoA merged commit 1fafaf5 into main Sep 28, 2023
9 checks passed
@RiscadoA RiscadoA deleted the 484-add-dictionarytrait branch September 28, 2023 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Define reflection for std::unordered_map<K, V> Define reflection for std::map<K, V> Add DictionaryTrait
2 participants