[feat] implement aliases#262
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #262 +/- ##
==========================================
+ Coverage 90.67% 90.74% +0.06%
==========================================
Files 17 17
Lines 3326 3435 +109
Branches 500 531 +31
==========================================
+ Hits 3016 3117 +101
- Misses 310 318 +8
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Signed-off-by: Anton Dukhovnikov <antond@wetafx.co.nz>
| path.append( "aliases.json" ); | ||
|
|
||
| if ( std::filesystem::exists( path ) ) | ||
| { |
There was a problem hiding this comment.
returning true when path doesn't exist is expected?
There was a problem hiding this comment.
This is an optional file, it is not expected to be in every directory specified in the paths list.
| std::ifstream ifstream( path ); | ||
| if ( !ifstream.is_open() ) | ||
| { | ||
| std::cerr << "Warning: cannot open file " << path.string() |
There was a problem hiding this comment.
we do show warning when you can't open the path, but we don't dow it for when it doesn't exist in a first place. expected?
There was a problem hiding this comment.
Expected. This path is actually the only bit which is not covered in unit tests. Same with the spectral files. May want to come up with a way to test if an existing file can't be opened, like restricting file permissions, or something. Will leave this for later, as it is not directly related to this feature.
| continue; | ||
| } | ||
|
|
||
| nlohmann::json file_data = nlohmann::json::parse( ifstream ); |
There was a problem hiding this comment.
yep, might add a try
|
|
||
| if ( !mapping_data.contains( type ) ) | ||
| { | ||
| continue; |
There was a problem hiding this comment.
for some "skip" we show warning, for others we don't
There was a problem hiding this comment.
The section is optional, being omitted from an aliases files is a normal case. We just go to the next file.
| { | ||
| bool has_prefix = true; | ||
|
|
||
| if ( make.length() <= model.length() ) |
There was a problem hiding this comment.
I am very confused about this logic. If it is correct we should leave comment here.
I've looked at the test and I still don't get it it =))
It is very specific, logic in this method. Has a lot of assumptions and build in heuristics. And it is not very clear. Comment would def. be needed.
There was a problem hiding this comment.
It is trivial really. Skip the prefix if it matches the make name, or the make name + whitespace.
"Canon" + "Canon R5" = "Canon R5"
"Canon R5" + "Canon R5" = "Canon R5"
"Canon" + "Canon_R5" = "Canon Canon_R5"
Signed-off-by: Anton Dukhovnikov <antond@wetafx.co.nz>
Signed-off-by: Anton Dukhovnikov <antond@wetafx.co.nz>
3 participants
Description
Implements the camera aliases functionality.
Depends on AcademySoftwareFoundation/rawtoaces-data#25
Tests
There is full test coverage.
Checklist:
need to update the documentation, for example if this is a bug fix that
doesn't change the API.)
(adding new test cases if necessary).
already run clang-format before submitting, I definitely will look at the CI
test that runs clang-format and fix anything that it highlights as being
nonconforming.