-
Notifications
You must be signed in to change notification settings - Fork 331
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
Allow for commit prefix #160
base: dev
Are you sure you want to change the base?
Conversation
… generated message feat(config.ts): add prefix configuration option to be used in the generated message. Add validation to ensure prefix is not empty.
…x and message content fix(config.ts): fix config validation for prefix to allow non-empty values
…fig.prefix is undefined instead of falsy
…egex pattern in git branch name feat(api.ts): add support for generating commit message prefix from git branch name if regex pattern is not provided feat(api.ts): add getCurrentGitBranch function to get current git branch name feat(api.ts): add generatePrefixFromRegex function to generate prefix from regex pattern in git branch name
resolve: #152 |
Improve generatePrefixFromRegex function to handle capturing groups Co-authored-by: Takuya Ono <[email protected]>
…c to improve security and reliability refactor(api.ts): simplify prefix generation logic by checking if prefix is defined and not empty before using it fix(api.ts): fix conditional statement in generatePrefixFromRegex to return undefined when no match is found fix(api.ts): catch error in getCurrentGitBranch and log it to console instead of throwing it
@takuya-o Thanks, added suggestion and changed to execa (+ minor change in handling undefined prefix) |
…utter dependencies list
@amyu98 Amazing, waiting for it to be released. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed that it has changed to execa
Any updates on this feature? |
@czlowiek488 Not quite sure who is supposed to push this towards merge. Do I need to do anything? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, sorry foe being late, have you tested the code?
To test you run: npm run build && npm run start
inside the repo, so it runs actual build files and not oc
which is installed globally
src/commands/config.ts
Outdated
[CONFIG_KEYS.prefix](value: any) { | ||
validateConfig( | ||
CONFIG_KEYS.prefix, | ||
true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true is not validating anything here :) we should validate the config input with a function or expression which return Boolean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@di-sukharev Do you have an idea what can we validate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you somehow need to confirm that the value is the exact format you expect, if your format is ^(*)
and not /^(*)/
— make sure you don't have /
s in the value, you may also do multiple validations like !a && b && !c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, sorry for being late :) Great work!
have you tested the code?
To test you run: npm run build && npm run start
inside the repo, so it runs actual build files and not oc
which is installed globally
and there are some conflicts to be fixed
Any news on getting this merged? Would be awesome 🚀 |
@tomtastico i hope soon, we need to make cc @amyu98 |
…variable name feat(config.ts): add validation for OCO_PREFIX environment variable to ensure it is a valid string or regex pattern
…g regex instead of startsWith and endsWith methods
…o a separate function to improve readability and maintainability feat(api.ts): add prefix to the message returned by getOpenCommitLatestVersion function to improve semantics and consistency with other functions
…allowForCommitPrefix
@di-sukharev |
@di-sukharev Do we want anything else done? |
How is this PR going? @di-sukharev Do we need something more? |
going to merge now, finally, sorry, was reallyy busy :) btw we won GitHub 2023 hackathon with the OpenCommit, congrats <3 |
|
||
function getCurrentGitBranch(): string | undefined { | ||
try { | ||
const branchName = execaCommandSync('git symbolic-ref --short HEAD').stdout.trim() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this going to add the full branch name in the commit like: #234-some-branch-name commit message text goes here
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it will take a regex out of the name. See my first commit on this PR.
Very cool to hear about the hackathon!
added 1 comment, could you also pls take a look at this PR, isn't it what you want here, but less code and more agile? |
Hmm the other PR might do the trick 😓 @di-sukharev |
But the other PR does not extract the number automatically. You would have to pass the placeholder each time a commit is made. |
Stale pull request message |
Any news on getting this merged? |
managing to merge this today, i need to fix the conflicts first |
@amyu98 if you have time today, maybe it would be faster for you to solve the conflicts :) im managing other PRs at the same time, if you chill today as we all should—will fix myself, not sure if i have so much time, there is lots of work to do.. |
I'll do my best to find the time
…On Sun, 3 Sept 2023, 9:08 Sukharev, ***@***.***> wrote:
@amyu98 <https://github.com/amyu98> if you have time today, maybe it
would be faster for you to solve the conflicts :)
im managing other PRs at the same time, if you chill today as we all
should—will fix myself, not sure if i have so much time, there is lots of
work to do..
—
Reply to this email directly, view it on GitHub
<#160 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ANXHFIFTKLOY4M65ZU5RKZTXYQNEXANCNFSM6AAAAAAXWAW7QA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Stale pull request message |
A new configuration option called "prefix" has been added. This option allows you to set a default prefix for each commit. It also supports regular expressions (regex) to match the prefix against the branch name.
Here is a simple example of using the prefix option:
oc config set prefix amyu98-commit:
This will result in:
amyu98-commit: Some lovely AI commit content
Below is an example of using a basic regex with the prefix option:
oc config set prefix /^([a-zA-Z]+-\d+)/
For the local branch "DEV-12345-commit-prefix", this will result in:
DEV-12345: Some lovely AI commit content