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

[config] ecal api config path review #1869

Closed
wants to merge 29 commits into from

Conversation

Peguen
Copy link
Contributor

@Peguen Peguen commented Dec 20, 2024

Description

Path processing and API changes regarding configuration and log paths.

API Changes

ecal_util.h

  • Renamed:
    • GeteCALCOnfigPath() -> GeteCALConfigDir()
    • GeteCALLogPath() -> GeteCALLogDir()
  • Removed:
    • GeteCALActiveIniFile() -> use config object GetYamlFilePath() instead
    • GeteCALUserSettingsPath() (cfg path in data path)

Logic Changes

  • GeteCALConfigDir() -> Checking for ecal.yaml in order: env(ECAL_CONFIG_DIR), local user paths (appdata/local, ~/.ecal) or system paths (etc/ecal, ProgramData/eCAL) ==> if nothing else specified, the default location of eCAL config dir will be the local user space
  • GeteCALLogDir() -> Same as GeteCALConfigDir(), env(ECAL_LOG_DIR) first and as fallback taking system temp path or as last resort /ecal_tmp path
  • Path returns will not simply create paths if they are not available. This now needs to be handled by the caller of the functions.

Misc

  • Log filter default in config/logging.h removed
  • Splitted path handling in new header/source file (previous was in config_initializer)
  • Using ecal-util functions for path processing
  • Added a functional test for ecal-util

@Peguen Peguen added the cherry-pick-to-NONE Don't cherry-pick these changes label Jan 2, 2025
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 made some suggestions

There were too many comments to post at once. Showing the first 20 out of 24. Check the log or trigger a new build to see more.

ecal/core/cfg/gen/generate_configuration_yaml.cpp Outdated Show resolved Hide resolved
ecal/core/cfg/gen/generate_configuration_yaml.cpp Outdated Show resolved Hide resolved
ecal/core/cfg/gen/generate_configuration_yaml.cpp Outdated Show resolved Hide resolved
ecal/core/cfg/gen/generate_configuration_yaml.cpp Outdated Show resolved Hide resolved
ecal/core/src/config/ecal_path_processing.cpp Outdated Show resolved Hide resolved
ecal/core/src/config/ecal_path_processing.cpp Outdated Show resolved Hide resolved
ecal/core/src/config/ecal_path_processing.cpp Outdated Show resolved Hide resolved
ecal/core/src/config/ecal_path_processing.cpp Outdated Show resolved Hide resolved
ecal/core/src/config/ecal_path_processing.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 made some suggestions

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 made some suggestions

#endif
}

std::string eCALPlatformSpecificFolder(const std::string& path_, const std::string& linux_folder_name_ = ECAL_FOLDER_NAME_HOME_LINUX, const std::string& win_folder_name_ = ECAL_FOLDER_NAME_WINDOWS)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: parameter 'win_folder_name_' is unused [misc-unused-parameters]

Suggested change
std::string eCALPlatformSpecificFolder(const std::string& path_, const std::string& linux_folder_name_ = ECAL_FOLDER_NAME_HOME_LINUX, const std::string& win_folder_name_ = ECAL_FOLDER_NAME_WINDOWS)
std::string eCALPlatformSpecificFolder(const std::string& path_, const std::string& linux_folder_name_ = ECAL_FOLDER_NAME_HOME_LINUX)

}
else
{
std::cout << "[eCAL] Log path does not exist: " << path << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: do not use 'std::endl' with streams; use '\n' instead [performance-avoid-endl]

Suggested change
std::cout << "[eCAL] Log path does not exist: " << path << std::endl;
std::cout << "[eCAL] Log path does not exist: " << path << '\n';

* ========================= eCAL LICENSE =================================
*/

#include <gmock/gmock.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'gmock/gmock.h' file not found [clang-diagnostic-error]

#include <gmock/gmock.h>
         ^


#include <cstdlib>

class ScopedEnvVar {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: class 'ScopedEnvVar' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

class ScopedEnvVar {
      ^

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 made some suggestions

const std::string env_ecal_log_value = "/path/to/log";

{ // Check for config path
MockEnvVar mock_env_var;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: variable 'mock_env_var' of type 'MockEnvVar' can be declared 'const' [misc-const-correctness]

Suggested change
MockEnvVar mock_env_var;
MockEnvVar const mock_env_var;

@Peguen
Copy link
Contributor Author

Peguen commented Jan 13, 2025

Closed due to merge problem and too many files to review.

@Peguen Peguen closed this Jan 13, 2025
@Peguen Peguen deleted the feature/ecal_api_config_path_review branch January 13, 2025 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-to-NONE Don't cherry-pick these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant