feat: refactor `Version.bump()` to accept bumping `major/minor/patch/last` #452

hadim · 2023-12-19T18:50:41Z

See #449 for context.

add new enum VersionBumpType
refactor Version.bump() to Version.bump(bump_type: VersionBumpType)
add unit tests
adapt the python binding

This PR is breaking backward compat but since rust does not support overloading or default args, I could not find a more creative way to keep backward compat. That being said, it might not be that much of a problem. If it is, please let me know, and I can always keep bump() as it is and create a new function.

…the bumping logic.

hadim · 2023-12-19T18:51:37Z

I would also need some help/guidance for the python binding.

crates/rattler_conda_types/src/version/mod.rs

baszalmstra · 2023-12-19T20:03:36Z

I would also need some help/guidance for the python binding.

@Wackyator ?

crates/rattler_conda_types/src/version/bump.rs

Wackyator · 2023-12-20T09:37:24Z

Hey @hadim, this looks great, appreciate the PR! I can help you with the Python bindings if you have any kind of issues.

hadim · 2023-12-20T10:02:45Z

Hey @hadim, this looks great, appreciate the PR! I can help you with the Python bindings if you have any kind of issues.

I am not sure how to 1) propagate the Result and the Error 2) integrate the new enum VersionBumpType

Wackyator · 2023-12-20T12:38:30Z

Hey @hadim, this looks great, appreciate the PR! I can help you with the Python bindings if you have any kind of issues.

I am not sure how to 1) propagate the Result and the Error 2) integrate the new enum VersionBumpType

For propagating the Result and Error, you can return the PyResult type from functions & extend the PyRattlerError and create the exception for it.
After that you can simply do a PyRattlerError::from the errors.
You can see an example of it here

For the VersionBumpType I'd say introduce seperate functions for them instead of making an enum on the python side, that seems more pythonic.

hadim · 2023-12-20T14:24:56Z

For the VersionBumpType I'd say introduce seperate functions for them instead of making an enum on the python side, that seems more pythonic.

Make sense! Should I keep bump() and forward it to bump_last()? or should I just get rid of it?

hadim · 2023-12-20T15:10:40Z

@Wackyator @baszalmstra: LGTM - ready for another round of review. Thanks!

wolfv · 2023-12-20T15:23:22Z

crates/rattler_conda_types/src/version/bump.rs

+    /// Bump the patch version number.
+    Patch,
+    /// Bump the last  version number.
+    Last,


Can you add a Segment(uint) type? So that we can explicitly select the segment to bump?

We might also consider that bumping non-existent segments should be possible. Usually, all non-existent segments are implicit .0, so bumping the patch of a 1.0 could also result in 1.0.1.

Can you add a Segment(uint) type? So that we can explicitly select the segment to bump?

Good idea. Will do.

We might also consider that bumping non-existent segments should be possible. Usually, all non-existent segments are implicit .0, so bumping the patch of a 1.0 could also result in 1.0.1.

Don't you prefer to instead fail, so it also builds incentive for the user to explicitly use x.x.x instead of x or x.x?

Unless you feel strongly here, I would prefer to let it for another PR (I can open a ticket if you want).

Added support for VersionBumpType::Segment(i32) in 9283770 (rust side only).

wolfv · 2023-12-20T15:25:00Z

py-rattler/src/version/mod.rs

-        Self {
-            inner: self.inner.bump(),
+    /// Returns a new version where the major segment of this version has been bumped.
+    pub fn bump_major(&self) -> PyResult<Self> {


If we had that integer option we could pass an int from Python (with -1 being the last element, for example), and bump like that.
Then in Python we could also define an enum for the different named segments and allow both, passing an enum or an int.

I am fine with this Python API.

What we have so far is:

bump_major()

bump_minor()

bump_patch()

bump_last()

I can add the below as well:

bump_segment(int)

bump(enum)

Does that sound good to you?

Python side done at 4ac1858

I didn't implement bump(enum) since it seems a bit overkill at this point, but let me know if you still want it.

wolfv · 2023-12-20T15:25:24Z

Really nice work. I had some simple remarks. Let me know what you think!

Wackyator

Good work, just a couple of nits. :)

py-rattler/src/version/mod.rs

py-rattler/tests/unit/test_version.py

Co-authored-by: Tarun Pratap Singh <[email protected]>

hadim · 2023-12-21T13:15:23Z

All good @Wackyator on my side.

Out of curiosity: are you using VSCode and rust-analyzer by any chance? I'm asking because I do and noticed that any .rs files located within py-rattler/ are failing to do any kind of linting/problem-detection within the VSCode editor, while all the .rs files located into crates/ works just fine. I tried playing with members = ["crates/*", "py-rattler/*"] in Cargo.toml but no success.

Sorry, I know it's not really related to that PR, but didnt want to open a new issue for this.

Wackyator · 2023-12-21T15:18:57Z

Out of curiosity: are you using VSCode and rust-analyzer by any chance? I'm asking because I do and noticed that any .rs files located within py-rattler/ are failing to do any kind of linting/problem-detection within the VSCode editor, while all the .rs files located into crates/ works just fine. I tried playing with members = ["crates/*", "py-rattler/*"] in Cargo.toml but no success.

Sorry, I know it's not really related to that PR, but didnt want to open a new issue for this.

I'm indeed using vscode and rust-analyzer for py-rattler, I usually open rattler/py-rattler/ in a separate vscode instance & it works fine.

Wackyator · 2023-12-21T15:22:16Z

This looks good to me, others can point out anything I may have missed.

hadim · 2023-12-21T15:25:32Z

I'm indeed using vscode and rust-analyzer for py-rattler, I usually open rattler/py-rattler/ in a separate vscode instance & it works fine.

Ok, thanks. I was trying to avoid that and after going into the rabbit hole I found that:

{
    "rust-analyzer.linkedProjects": [
        "./py-rattler/Cargo.toml"
    ]
}

into .vscode/settings.json solve the issue.

baszalmstra

I left one final comment with regards to the unit test but other than that this looks good to me!

py-rattler/tests/unit/test_version.py

Add new VersionBumpType and refactor Version.bump() to determine …

6bdf0f2

…the bumping logic.

hadim force-pushed the bump branch from f2218d0 to 6bdf0f2 Compare December 19, 2023 18:51

remove left over default for bump type

6748325

hadim mentioned this pull request Dec 19, 2023

Extend version.bump() to allow bumping minor|major|patch #449

Closed

baszalmstra reviewed Dec 19, 2023

View reviewed changes

crates/rattler_conda_types/src/version/mod.rs Show resolved Hide resolved

crates/rattler_conda_types/src/version/mod.rs Outdated Show resolved Hide resolved

Return an error instead of panic

4d37df4

baszalmstra reviewed Dec 19, 2023

View reviewed changes

crates/rattler_conda_types/src/version/bump.rs Outdated Show resolved Hide resolved

hadim added 2 commits December 19, 2023 16:25

use thiserror for VersionBumpError

93036bc

wip attempt for the python binding

2228f1c

hadim and others added 2 commits December 20, 2023 09:54

Python binding wrap up

f4ea4c6

Merge branch 'main' into bump

fd7f90a

wolfv reviewed Dec 20, 2023

View reviewed changes

hadim added 5 commits December 20, 2023 11:34

introduce VersionBumpType::Segment(i32)

9283770

python binding for bump_segment

4ac1858

lint

90cd9ce

cleaning

951fbc8

more cleaning

87bd09b

Wackyator suggested changes Dec 21, 2023

View reviewed changes

hadim and others added 3 commits December 21, 2023 06:45

Merge branch 'main' into bump

2c09d87

Update py-rattler/src/version/mod.rs

3c85dba

Co-authored-by: Tarun Pratap Singh <[email protected]>

Refactor version bumping methods in PyVersion

d3a6362

Remove unused test_bump function

4d77802

hadim requested a review from Wackyator December 21, 2023 13:12

Wackyator approved these changes Dec 21, 2023

View reviewed changes

baszalmstra requested a review from wolfv January 2, 2024 08:40

baszalmstra reviewed Jan 2, 2024

View reviewed changes

py-rattler/tests/unit/test_version.py Outdated Show resolved Hide resolved

hadim and others added 3 commits January 2, 2024 09:32

Merge branch 'main' into bump

1f02dff

move tests to doctests

a471b0c

cleaning

5f6e070

baszalmstra approved these changes Jan 2, 2024

View reviewed changes

baszalmstra merged commit 02e68c9 into conda:main Jan 2, 2024
13 checks passed

hadim deleted the bump branch January 2, 2024 15:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: refactor `Version.bump()` to accept bumping `major/minor/patch/last` #452

feat: refactor `Version.bump()` to accept bumping `major/minor/patch/last` #452

hadim commented Dec 19, 2023 •

edited

Loading

hadim commented Dec 19, 2023

baszalmstra commented Dec 19, 2023

Wackyator commented Dec 20, 2023

hadim commented Dec 20, 2023

Wackyator commented Dec 20, 2023 •

edited

Loading

hadim commented Dec 20, 2023

hadim commented Dec 20, 2023

wolfv Dec 20, 2023

hadim Dec 20, 2023

hadim Dec 20, 2023

hadim Dec 20, 2023

wolfv Dec 20, 2023

hadim Dec 20, 2023 •

edited

Loading

hadim Dec 20, 2023

wolfv commented Dec 20, 2023

Wackyator left a comment

hadim commented Dec 21, 2023

Wackyator commented Dec 21, 2023

Wackyator commented Dec 21, 2023

hadim commented Dec 21, 2023

baszalmstra left a comment

feat: refactor Version.bump() to accept bumping major/minor/patch/last #452

feat: refactor Version.bump() to accept bumping major/minor/patch/last #452

Conversation

hadim commented Dec 19, 2023 • edited Loading

hadim commented Dec 19, 2023

baszalmstra commented Dec 19, 2023

Wackyator commented Dec 20, 2023

hadim commented Dec 20, 2023

Wackyator commented Dec 20, 2023 • edited Loading

hadim commented Dec 20, 2023

hadim commented Dec 20, 2023

wolfv Dec 20, 2023

Choose a reason for hiding this comment

hadim Dec 20, 2023

Choose a reason for hiding this comment

hadim Dec 20, 2023

Choose a reason for hiding this comment

hadim Dec 20, 2023

Choose a reason for hiding this comment

wolfv Dec 20, 2023

Choose a reason for hiding this comment

hadim Dec 20, 2023 • edited Loading

Choose a reason for hiding this comment

hadim Dec 20, 2023

Choose a reason for hiding this comment

wolfv commented Dec 20, 2023

Wackyator left a comment

Choose a reason for hiding this comment

hadim commented Dec 21, 2023

Wackyator commented Dec 21, 2023

Wackyator commented Dec 21, 2023

hadim commented Dec 21, 2023

baszalmstra left a comment

Choose a reason for hiding this comment

feat: refactor `Version.bump()` to accept bumping `major/minor/patch/last` #452

feat: refactor `Version.bump()` to accept bumping `major/minor/patch/last` #452

hadim commented Dec 19, 2023 •

edited

Loading

Wackyator commented Dec 20, 2023 •

edited

Loading

hadim Dec 20, 2023 •

edited

Loading