Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

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

Merged

Conversation

jaysonsantos
Copy link
Contributor

@jaysonsantos jaysonsantos commented Mar 20, 2021

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.toml as 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 like Cargo.lock:version:project-name where it only bumps version if the project name was found before it.

Checklist

  • Add test cases to all the changes you introduce
  • Run ./script/format and ./script/test locally to ensure this change passes linter check and test
  • Test the changes on the local machine manually
  • Update the documentation for the changes

Expected behavior

Bumping versions on Cargo.lock or 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.

@codecov
Copy link

codecov bot commented Mar 20, 2021

Codecov Report

Merging #361 (b7bf451) into master (ed06664) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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              
Flag Coverage Δ
unittests 97.58% <100.00%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
commitizen/cli.py 97.50% <ø> (ø)
commitizen/__version__.py 100.00% <100.00%> (ø)
commitizen/bump.py 100.00% <100.00%> (ø)
commitizen/commands/bump.py 95.19% <100.00%> (+0.74%) ⬆️
commitizen/out.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e2c7303...b7bf451. Read the comment docs.

@jaysonsantos jaysonsantos force-pushed the version-in-random-position branch 2 times, most recently from 6632b45 to f129001 Compare March 20, 2021 23:04
@Lee-W
Copy link
Member

Lee-W commented Mar 21, 2021

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

@jaysonsantos
Copy link
Contributor Author

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?

@woile
Copy link
Member

woile commented Mar 21, 2021

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 💪🏻

@Lee-W
Copy link
Member

Lee-W commented Mar 22, 2021

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 🙂

@jaysonsantos
Copy link
Contributor Author

jaysonsantos commented Mar 22, 2021

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

@jaysonsantos jaysonsantos force-pushed the version-in-random-position branch from f129001 to 4ee04dc Compare March 22, 2021 20:42
@jaysonsantos
Copy link
Contributor Author

Hey there folks, check the latest commit I sent to see the implementation 4ee04dc

@jaysonsantos
Copy link
Contributor Author

Sorry, I forgot that bumpversion uses a bunch of python versions and I used the walrus operator

@jaysonsantos jaysonsantos force-pushed the version-in-random-position branch from 4ee04dc to 9a02d77 Compare March 22, 2021 20:50
@jaysonsantos
Copy link
Contributor Author

Ok, no walrus operator now

@jaysonsantos jaysonsantos force-pushed the version-in-random-position branch from 9a02d77 to e8841ec Compare March 22, 2021 20:58
@Lee-W
Copy link
Member

Lee-W commented Mar 23, 2021

@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.

@jaysonsantos
Copy link
Contributor Author

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?

@Lee-W
Copy link
Member

Lee-W commented Mar 26, 2021

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.

@jaysonsantos
Copy link
Contributor Author

@Lee-W you can take a look again

Copy link
Member

@Lee-W Lee-W left a 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 😃

@Lee-W Lee-W merged commit 8689546 into commitizen-tools:master Apr 2, 2021
@jaysonsantos jaysonsantos deleted the version-in-random-position branch April 17, 2021 16:31
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.

3 participants