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

Implement #139: Bump version inside VersionInfo #141

Merged
merged 3 commits into from
Sep 30, 2019

Conversation

tomschr
Copy link
Member

@tomschr tomschr commented Sep 30, 2019

This PR implements #139

  • Add bump_major, bump_minor, bump_patch, bump_prerelease and bump_build as methods in class VersionInfo. With this methods, it is now possible to write something like this:

    ver = semver.parse_version_info("3.4.5")
    new_ver = ver.bump_major().bump_minor()
  • Add test cases


Some comments about this implementation:

  • I (re)used the module functions semver.bump_major() etc. to avoid implementing the same code (double code is bad code).
  • It may be not as efficient as it could be, because it converts the VersionInfo into a string, bumps it, and convert is back into VersionInfo. Not sure if there is a better way to do it.
  • I've created a "Draft pull request" so we can discuss and improve the code. ;-)

@ppkt Karol, what do you think? :-)

* Add bump_major, bump_minor, bump_patch, bump_prerelease and bump_build
  as methods in class VersionInfo. With this methods, it is now possible
  to write something like this:

```python
ver = semver.parse_version_info("3.4.5")
new_ver = ver.bump_major().bump_minor()
```

* Add test cases
Copy link
Member

@ppkt ppkt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for quick PR :) I have only small remarks regarding docstrings

semver.py Outdated Show resolved Hide resolved
semver.py Outdated Show resolved Hide resolved
@tomschr tomschr marked this pull request as ready for review September 30, 2019 10:39
@tomschr
Copy link
Member Author

tomschr commented Sep 30, 2019

@ppkt I've implemented your suggestion in the above commits. Hope you are fine with that. 😉 Let me know if you prefer to squash the commits.

By the way: what amount of lines do I need to change to qualify for the contributors file? 😉

@tomschr
Copy link
Member Author

tomschr commented Sep 30, 2019

Another question about consistency.

I've found out that some docstring are written differently. Some docstrings have this style:

def parse(version):
    """Parse version to major, minor, patch, pre-release, build parts.

    :param version: version string
    :return: dictionary with the keys 'build', 'major', 'minor', 'patch',
             and 'prerelease'. The prerelease or build keys can be None
             if not provided
    :rtype: dict

    >>> import semver
    >>> ver = semver.parse('3.4.5-pre.2+build.4')
    >>> # [...]
    """

while others have this style:

def bump_major(self):
        """Raise the major part of the version, return a new object
           but leave self untouched

        :return: new object with the raised major part
        :rtype: VersionInfo

        >>> import semver
        >>> ver = semver.parse_version_info("3.4.5")
        >>> ver.bump_major()
        VersionInfo(major=4, minor=0, patch=0, prerelease=None, build=None)
        """

Probably that was me to blame. 😢 To summarize: either we write doctests first and then all parameters, or vice versa. Is there any preferred style?

IMHO, at least we should be consistent. I would like to fix that, but that's probably something for a different PR, right?

@scls19fr scls19fr merged commit a598059 into python-semver:master Sep 30, 2019
@scls19fr
Copy link
Member

Thanks @tomschr for this PR and @ppkt for this review.

@tomschr tomschr deleted the feature/139-bump-versioninfo branch September 30, 2019 18:30
@ppkt
Copy link
Member

ppkt commented Sep 30, 2019

@tomschr I think you can update docstrings in separate PR. Regarding "contributors" file - I don't think there is any limit - feel free to add yourself. And thank you for your PRs :)

@tomschr
Copy link
Member Author

tomschr commented Sep 30, 2019

@ppkt Ok, will attack consistency of docstrings in another PR. 😉 Regarding contributors file, I will add myself in one of my open PRs. 😄 Thanks!

@scls19fr
Copy link
Member

scls19fr commented Oct 1, 2019

This PR helps this quite old issue: #40

@scls19fr scls19fr mentioned this pull request Oct 1, 2019
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.

None yet

3 participants