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

corrade-rc: allow shorthand filename specification #163

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mosra
Copy link
Owner

@mosra mosra commented Jan 1, 2023

Thought this was a quickie to whip up after 9ef4b4a but ultimately it wasn't, as always, and I ran out of time allocated for "random stuff".


If no alias, nullTerminated or align options are needed or the global values are sufficient, having to write a [file] section for each file is overly verbose. This allows to specify filename options directly in the global configuration section.

It's filename and not file in order to not have to further complicate the CMake parsing routine that extract dependency information from the file -- this way it doesn't even need to be changed.

Things left to do:

  • while basic support for this was easy enough, the filesystem group override code needs to be adapted and tested for this, which involves copying the file somewhere temporary and changing the contents (because we can't make use of an alias in this scenario)
  • there's also a newly uncovered bug, see the TODO in Resource.cpp
  • this also needs to be documented

If no alias, nullTerminated or align options are needed or the global
values are sufficient, having to write a [file] section for each file is
overly verbose. This allows to specify filename= options directly in the
global configuration section.

It's filename= and not file= in order to not have to further complicate
the CMake parsing routine that extract dependency information from the
file -- this way it doesn't even need to be changed.

TODO: while basic support for this was easy enough, the filesystem group
  override code needs to be adapted and tested for this, which involves
  copying the file somewhere temporary and changing the contents
  (because we can't make use of an alias in this scenario)
TODO: there's also a newly uncovered bug, see the TODO in Resource.cpp
TODO: this also needs to be documented
@codecov
Copy link

codecov bot commented Jan 2, 2023

Codecov Report

Base: 97.98% // Head: 97.93% // Decreases project coverage by -0.05% ⚠️

Coverage data is based on head (77e8588) compared to base (b317b8a).
Patch coverage: 70.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #163      +/-   ##
==========================================
- Coverage   97.98%   97.93%   -0.06%     
==========================================
  Files         136      136              
  Lines       11151    11170      +19     
==========================================
+ Hits        10926    10939      +13     
- Misses        225      231       +6     
Impacted Files Coverage Δ
src/Corrade/Utility/Resource.cpp 88.46% <33.33%> (-7.20%) ⬇️
...c/Corrade/Utility/Implementation/ResourceCompile.h 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

1 participant