-
-
Notifications
You must be signed in to change notification settings - Fork 301
feat: Support versions on random positions #361
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
feat: Support versions on random positions #361
Conversation
Codecov Report
@@ Coverage Diff @@ ## master #361 +/- ## ========================================== + Coverage 97.53% 97.58% +0.04% ========================================== Files 39 39 Lines 1340 1365 +25 ========================================== + Hits 1307 1332 +25 Misses 33 33
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
6632b45 to
f129001
Compare
Hi @jaysonsantos, thanks for this PR. This one looks like a rust-specific solution to me. I'm thinking of using regex like name = "there-i-fixed-it"\nversion = "\d\.\d\.\d\."'. But currently, we're paring it by line. We might need to re-implement this method
Hey there @Lee-W that was my first try and then I found out it was line based.
What if I change it to regex replace on the whole file with limit of one?
Hey thanks for the contribution, this looks like a nice feature to have.
I think a re-implementation of the line based regex would be really cool , but we should not forget the no regex case.
Looking forward to see it 💪🏻
I don't have a clear idea of how this should be implemented at this moment. Maybe we could discuss more on this thread before we implement the new version 🙂
One of the ways I see this working is:
- Read the whole file contents into a variable
- Search with a regex (MULTILINE) so you get the start-end of the match
- Seek to the end of the match, get everything from there until the next \n or eof
- Replace only between the end of the match until \n or eof
f129001 to
4ee04dc
Compare
Hey there folks, check the latest commit I sent to see the implementation 4ee04dc
Sorry, I forgot that bumpversion uses a bunch of python versions and I used the walrus operator
4ee04dc to
9a02d77
Compare
Ok, no walrus operator now
9a02d77 to
e8841ec
Compare
@jaysonsantos Thanks for your immediate update 🎉 The behavior is slightly changed (update all v.s. update the last one). I'm not sure whether we should consider it as a breaking change. We might need to describe it in our documentation clearly. Also, I'm thinking of displaying a warning if there're multiple regex matches.
The behavior is slightly changed (update all v.s. update the last one).
I didn't get I think it is the same logic, no?
The old one would loop through all lines and replace exact matches of the old version to the new one. This https://github.com/commitizen-tools/commitizen/pull/361/files#diff-ed9bead31489109a8441fbfb00d2ddec00198489b0702bab164825c2d3bd9b4fR191 will be even more strict as it will only update if the exact version is surrounded by boundaries, so v1.2.3 wouldn't be changed.
I THINK it does not break compatibility, if you can think of a test, can you send me and I add a case for it?
The behavior is slightly changed (update all v.s. update the last one).
I didn't get I think it is the same logic, no?
The old one would loop through all lines and replace exact matches of the old version to the new one. This https://github.com/commitizen-tools/commitizen/pull/361/files#diff-ed9bead31489109a8441fbfb00d2ddec00198489b0702bab164825c2d3bd9b4fR191 will be even more strict as it will only update if the exact version is surrounded by boundaries, so v1.2.3 wouldn't be changed.
I THINK it does not break compatibility, if you can think of a test, can you send me and I add a case for it?
I'm actually ok with it. Just raise this issue as I was not yet familiar enough with the changes.
@Lee-W you can take a look again
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.
Looks good to me 😃
Uh oh!
There was an error while loading. Please reload this page.
First thanks for this quite good project!
Description
This is a rather confusing logic but I decided to send this PR as a way to start the conversation.
I am trying to use commitizen to bump rust projects and its default way works fine when bumping the version inside a
Cargo.tomlas usually, your own package's version is on the top. But if you have a lock file, rust also bumps the version there. This PR was done in a way to bump the version only after something is matched, in the example, I used the project name so the versions_file could have something likeCargo.lock:version:project-namewhere it only bumps version if the project name was found before it.Checklist
./script/formatand./script/testlocally to ensure this change passes linter check and testExpected behavior
Bumping versions on
Cargo.lockor any other file, should bump only the intended line.Steps to Test This Pull Request
Try to bump any Cargo.lock file or any file that can have your own version, randomly placed in it.