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

refactoring v2 #408

Merged
merged 12 commits into from
Sep 6, 2024
Merged

refactoring v2 #408

merged 12 commits into from
Sep 6, 2024

Conversation

di-sukharev
Copy link
Owner

@di-sukharev di-sukharev commented Sep 4, 2024

migrates all API_KEYs configs (e.g. OCO_OPENAI_API_KEY, OCO_GEMINI_API_KEY) to a single OCO_API_KEY config — to make it simpler to support.

same for OCO_ENDPOINT_URLs and OCO_API_URLs, there were a bunch, now it's all a single OCO_API_URL.

migrations should manage changing old keys to new keys automatically.

… to FlowiseEngine, GeminiEngine, and OllamaEngine for consistency and clarity

fix(engine): update imports and instantiation of renamed classes in engine utility functions to ensure proper functionality
…guration for better clarity and usability

feat(migrations): implement migration to consolidate API key and URL into a single configuration for improved simplicity
refactor(config): rename configuration keys to use a single API key and URL for all LLM providers, enhancing consistency
fix(engine): update engine initialization to use new unified API key and URL configuration
test(config): update tests to validate new configuration structure and ensure backward compatibility with existing setups
feat(migrations): add migration to remove obsolete config keys from global file to streamline configuration management
…e.json

fix(commit.ts): update success message to start with a lowercase letter for consistency
…OKENS_MAX_OUTPUT to ensure proper configuration

refactor(generateCommitMessageFromGitDiff.ts): simplify MAX_TOKENS_INPUT and MAX_TOKENS_OUTPUT assignments by removing redundant default value logic
…y and clarity

fix(config): add validation for OCO_WHY configuration key to ensure it is a boolean
refactor(config): extract default config setting logic into setDefaultConfigValues function
fix(cli): reorder function calls to ensure checkIsLatestVersion runs after runMigrations
chore(migrations): update getConfig calls to disable caching and default value setting
chore(migrations): remove obsolete configuration keys from global config file
fix(migrations): improve migration logging with consistent output formatting
style(migrations): enhance migration success and failure messages for better user feedback
…s to set to prevent unnecessary function calls

feat(run.ts): add conditional to skip migrations if OCO_AI_PROVIDER is set to TEST to improve migration handling during testing
@di-sukharev
Copy link
Owner Author

@matscube please help me here :) for some reason tests are not cleaning up the env config and each test conflicts with the previous test env config setup. when running with it.only() it works fine. what am i missing?

@matscube
Copy link
Contributor

matscube commented Sep 4, 2024

@di-sukharev
Hi, It seems that some tests fail due to the options of cache and setDefaultValues in getConfig.

export const getConfig = ({
  envPath = defaultEnvPath,
  globalPath = defaultConfigPath,
  cache = true, // here
  setDefaultValues = true // here
}: GetConfigOptions = {}): ConfigType => {
  • By default, cache is true, so the value of getConfig called in the same process is reused, causing the value of the previous test case to be mistakenly called and causing a conflict between tests. When running in Unit Test, most tests should be fixed by passing cache: false to getConfig.

  • setConfig may be called in setDefaultConfigValues called by getConfig, but the globalConfigPath option is not passed to setConfig.

const setDefaultConfigValues = (config: ConfigType) => {
  const entriesToSet: [key: string, value: string | boolean | number][] = [];
  for (const entry of Object.entries(DEFAULT_CONFIG)) {
    const [key, _value] = entry;
    if (config[key] === 'undefined') entriesToSet.push(entry);
  }

  if (entriesToSet.length > 0) setConfig(entriesToSet); // called without globalConfigPath
};

Therefore, the path of the temporary file created in Unit Test should be passed to getConfig or setConfig, but there seems to be a part where the processing is switched to refer to the default setting path (~/.opencommit) through the setDefaultConfigValues method.
Also, there is a concern that an infinite loop may occur between getConfig and setConfig through setDefaultConfigValues, so caution is required.
getConfig => setDefaultConfigValues => setConfig => getConfig

For example, changing the default values of cache and setDefaultValues from true to false, the test will pass.

export const getConfig = ({
  envPath = defaultEnvPath,
  globalPath = defaultConfigPath,
  cache = false, // changed
  setDefaultValues = false // changed
}: GetConfigOptions = {}): ConfigType => {

@di-sukharev
Copy link
Owner Author

100%, i see, thank you, solving

… clean up code

feat(config.ts): create getIsGlobalConfigFileExist function to check for config file existence
feat(migrations): add migration to set missing default values for configuration
fix(migrations): update migration functions to improve clarity and functionality
chore(migrations): register new migration for setting missing default values
style(migrations): format code for better readability in migration files
test(config.test.ts): update tests to improve readability and maintainability
@di-sukharev di-sukharev merged commit f975e49 into dev Sep 6, 2024
3 checks passed
di-sukharev added a commit that referenced this pull request Sep 6, 2024
* 378: fix hook env (#402)

* fix(prepare-commit-msg-hook): update error handling to provide clearer instructions for setting API keys and improve user guidance

* Fix: a bug that causes an error when pushing without setting git remote (#396)

* refactoring v2 (#408)

* 3.2.0

* update deploy commands

---------

Co-authored-by: Takanori Matsumoto <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants