-
Notifications
You must be signed in to change notification settings - Fork 57
More unit tests for util #192
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
Changes from all commits
e8ea9e5
a12aed5
6689447
2404161
285b2fa
5005ad8
8fa37b3
4d6610b
64a9683
8027ccb
bb207fe
422ad82
3b7d449
32fb368
416c5f5
9f4fc09
cd9b9d0
2642889
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,60 +50,65 @@ struct CameraIdentifier | |
| * @return true if the file was processed (either added to batch or filtered out), | ||
| * false if the file should be ignored | ||
| */ | ||
| bool check_and_add_file( | ||
| void check_and_add_file( | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. boolean is not used in any way |
||
| const std::filesystem::path &path, std::vector<std::string> &batch ) | ||
| { | ||
| bool is_regular_file = std::filesystem::is_regular_file( path ) || | ||
| std::filesystem::is_symlink( path ); | ||
| if ( !is_regular_file ) | ||
| { | ||
| std::cerr << "Not a regular file: " << path << std::endl; | ||
| return false; | ||
| return; | ||
| } | ||
|
|
||
| static const std::set<std::string> ignore_filenames = { ".DS_Store" }; | ||
| std::string filename = path.filename().string(); | ||
| if ( ignore_filenames.count( filename ) > 0 ) | ||
| return false; | ||
| return; | ||
|
|
||
| static const std::set<std::string> ignore_extensions = { ".exr", | ||
| ".jpg", | ||
| ".jpeg" }; | ||
| std::string extension = OIIO::Strutil::lower( path.extension().string() ); | ||
| if ( ignore_extensions.count( extension ) > 0 ) | ||
| return false; | ||
| return; | ||
|
|
||
| batch.push_back( path.string() ); | ||
|
|
||
| return true; | ||
| return; | ||
| } | ||
|
|
||
| bool collect_image_files( | ||
| const std::string &path, std::vector<std::vector<std::string>> &batches ) | ||
| std::vector<std::vector<std::string>> | ||
| collect_image_files( const std::vector<std::string> &paths ) | ||
| { | ||
| if ( !std::filesystem::exists( path ) ) | ||
| std::vector<std::vector<std::string>> batches( 1 ); | ||
|
|
||
| for ( const auto &path: paths ) | ||
| { | ||
| return false; | ||
| } | ||
| if ( !std::filesystem::exists( path ) ) | ||
| { | ||
| std::cerr << "File or directory not found: " << path << std::endl; | ||
| continue; | ||
| } | ||
|
|
||
| auto canonical_filename = std::filesystem::canonical( path ); | ||
| auto canonical_filename = std::filesystem::canonical( path ); | ||
|
|
||
| if ( std::filesystem::is_directory( path ) ) | ||
| { | ||
| std::vector<std::string> &curr_batch = batches.emplace_back(); | ||
| auto it = std::filesystem::directory_iterator( path ); | ||
| if ( std::filesystem::is_directory( path ) ) | ||
| { | ||
| std::vector<std::string> &curr_batch = batches.emplace_back(); | ||
| auto it = std::filesystem::directory_iterator( path ); | ||
|
|
||
| for ( auto filename: it ) | ||
| for ( auto filename: it ) | ||
| { | ||
| check_and_add_file( filename, curr_batch ); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| check_and_add_file( filename, curr_batch ); | ||
| check_and_add_file( path, batches[0] ); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| check_and_add_file( path, batches[0] ); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this was actually a bug cought by the unit test. Horray for unit tests.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what makes you think this was a bug? the code does exactly what was intended, see main.cpp#40
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. right. That is rather specific expectation. If it was private API and used internally, I'd see no problem with that. But expecting caller to know that expectation seems specific. Happy to revert and modify the test. You can imagine test is failing when
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that the split between main/util is not optimal here, perhaps the outer loop (main.cpp#44) should be also moved inside
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you are right. I didn't know it was intended behaviour. Now I know. I've refactored basd on your suggestion. Now there is no need for boolean return, so batches is the return now. I've added a test that checks that exact behaviour. |
||
| } | ||
|
|
||
| return true; | ||
| return batches; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can see how it is used later on in so, this ^ would deal with empty batches no problem, but from API perspective it doesn't make sense to have empty batch in return. So removing empty won't brake consumer, but make result cleaner.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does make sense though. It really depends on how the client needs the pipeline organised. Example:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok. I've reverted it back. Empty batches are back. |
||
| } | ||
|
|
||
| /// Gets the list of database paths for rawtoaces data files. | ||
|
|
@@ -1165,6 +1170,8 @@ std::vector<std::string> ImageConverter::supported_cameras() | |
| /// Normalise the metadata in the cases where the OIIO attribute name | ||
| /// doesn't match the standard OpenEXR and/or ACES Container attribute name. | ||
| /// We only check the attribute names which are set by the raw input plugin. | ||
| /// | ||
| /// @param spec ImageSpec to modify | ||
| void fix_metadata( OIIO::ImageSpec &spec ) | ||
| { | ||
| const std::map<std::string, std::string> standard_mapping = { | ||
|
|
@@ -1186,8 +1193,6 @@ void fix_metadata( OIIO::ImageSpec &spec ) | |
| { | ||
| if ( type.basetype == OIIO::TypeDesc::STRING ) | ||
| spec[dst_name] = src_attribute->get_string(); | ||
| else if ( type.basetype == OIIO::TypeDesc::FLOAT ) | ||
| spec[dst_name] = src_attribute->get_float(); | ||
| } | ||
| spec.erase_attribute( src_name ); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| // Copyright Contributors to the rawtoaces Project. | ||
|
|
||
| #pragma once | ||
|
|
||
| #include <filesystem> | ||
| #include <string> | ||
| #include <vector> | ||
| #include <OpenImageIO/imageio.h> | ||
|
|
||
| // Contains the declarations of the private functions, | ||
| // exposed here for unit-testing. | ||
|
|
||
| namespace rta | ||
| { | ||
| namespace util | ||
| { | ||
|
|
||
| std::vector<std::string> database_paths(); | ||
|
|
||
| void fix_metadata( OIIO::ImageSpec &spec ); | ||
|
|
||
| } // namespace util | ||
| } // namespace rta |
Uh oh!
There was an error while loading. Please reload this page.