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

3.2.0 #412

Merged
merged 5 commits into from
Sep 6, 2024
Merged

3.2.0 #412

merged 5 commits into from
Sep 6, 2024

Conversation

di-sukharev
Copy link
Owner

No description provided.

di-sukharev and others added 5 commits September 3, 2024 13:14
* fix(prepare-commit-msg-hook): update error handling to provide clearer instructions for setting API keys and improve user guidance

* build
@di-sukharev
Copy link
Owner Author

di-sukharev commented Sep 6, 2024

@matscube thank you for your help, gonna publish soon. a review would be much appreciated. tests pass and i tested in 10 times locally, but if you see a concern — pls lmk

@matscube
Copy link
Contributor

matscube commented Sep 6, 2024

@di-sukharev
I'm really grateful for this tool as a user, and I'm happy to be able to contribute as a developer.
I'm always looking forward to new features being added. 🎉

@di-sukharev di-sukharev merged commit 306522e into master Sep 6, 2024
5 checks passed
@di-sukharev
Copy link
Owner Author

@matscube deployed, please update and lmk if it works for you, there are some tricky migrations to the config happening as you can see in the code. i've deleted a lot of redundant config vars and made it simpler to support

@matscube
Copy link
Contributor

matscube commented Sep 6, 2024

@di-sukharev
Thank you for the deployment!
I installed the latest version of oco(3.2.1) and tried it out a bit, and found some points that might be better to fix.


When I ran the oco command, the migration ran. (I may have had to run it a few times.)

$ oco

After running the migration, I ran oco and was instructed to set OCO_AI_PROVIDER, so I set it to openai with the config command.

$ oco config set OCO_AI_PROVIDER=openai

Also, the migration caused the API_KEY (OCO_OPENAI_API_KEY) setting to disappear, so I set OCO_API_KEY again.

When I ran the oco command again, I got an error about max_tokens.

$ oco

✖ 400 Invalid type for 'max_tokens': expected an integer, but got a string instead.

By migration, the following line will be automatically set in the global configuration file, but it seems that an error will occur if it is undefined.

OCO_TOKENS_MAX_OUTPUT=undefined

If you comment out the line of OCO_TOKENS_MAX_OUTPUT, oco can be executed.


It seems that an error occurs when OCO_TOKENS_MAX_OUTPUT is undefined. (OCO_TOKENS_MAX_INPUT was fine even if it was undefined)

By commenting out OCO_TOKENS_MAX_OUTPUT or deleting the setting itself from .opencommit, oco can be executed.
However, when the oco config set command is executed, if there is no setting for OCO_TOKENS_MAX_OUTPUT, OCO_TOKENS_MAX_OUTPUT=undefined will be automatically added, so the user needs to set the appropriate number for OCO_TOKENS_MAX_OUTPUT by themselves.

Some users might get in trouble.
It might be better to set the value of OCO_TOKENS_MAX_OUTPUT automatically set by the oco config set command to 500, the default value, instead of undefined.

@di-sukharev
Copy link
Owner Author

i see, preparing the patch

@di-sukharev
Copy link
Owner Author

@matscube hmm.. i dont see this behaviour. for me src/migrations/02_set_missing_default_values.ts works correctly and sets the missing keys to their default values.

this is my config before migrations:

OCO_TOKENS_MAX_OUTPUT=undefined
OCO_MODEL=gpt-4o-mini
OCO_API_URL=undefined
OCO_OPENAI_API_KEY=my_key
OCO_AI_PROVIDER=openai
OCO_TOKENS_MAX_INPUT=undefined
OCO_DESCRIPTION=false
OCO_EMOJI=false
OCO_LANGUAGE=en
OCO_MESSAGE_TEMPLATE_PLACEHOLDER=$msg
OCO_PROMPT_MODULE=conventional-commit
OCO_ONE_LINE_COMMIT=false
OCO_TEST_MOCK_TYPE=commit-message
OCO_GITPUSH=true

this is the config after migrations were applied:

OCO_TOKENS_MAX_OUTPUT=4096
OCO_MODEL=gpt-4o-mini
OCO_API_URL=undefined
OCO_API_KEY=my_key
OCO_AI_PROVIDER=openai
OCO_TOKENS_MAX_INPUT=40960
OCO_DESCRIPTION=false
OCO_EMOJI=false
OCO_LANGUAGE=en
OCO_MESSAGE_TEMPLATE_PLACEHOLDER=$msg
OCO_PROMPT_MODULE=conventional-commit
OCO_ONE_LINE_COMMIT=false
OCO_TEST_MOCK_TYPE=commit-message
OCO_GITPUSH=true

@di-sukharev
Copy link
Owner Author

i reproduced the behaviour. thank you

@di-sukharev di-sukharev mentioned this pull request Sep 7, 2024
@di-sukharev
Copy link
Owner Author

will be fixed here #414

@di-sukharev
Copy link
Owner Author

merged #413

@matscube
Copy link
Contributor

I confirmed that it works properly. Thank you!

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