Decide to deprecate module level functions? #225

tomschr · 2020-02-29T17:48:27Z

Situation

Currently, we have mostly functions which are either on the module level (semver.next_version, semver.bump_*) and almost the same on the VersionInfo level (semver.VersionInfo.next_version, semver.VersionInfo.bump_*).

I know, they both serve distinctive purposes: the first expects and returns a string whereas the second needs a VersionInfo object.

However, from an implementation perspective, I'm wondering if this is double work. It looks inefficient to me when the VersionInfo methods need to convert the object first into a string, call the modul level function, and convert it back to a VersionInfo object.

IMHO, we should improve this situation.

Suggested Solution

The question boils down to this: should we treat VersionInfo as first class object?

I have the impression this isn't currently the case. The developer can decide between two implementation which does mostly the same. The only difference is, one implementation use strings, the other use a "detour" with an VersionInfo object.

If we would treat VersionInfo as our one and only solution, that would mean we have to move the implementation into the methods. The module level functions would serve as "helper functions".

Or... maybe we could think about a more radical approach? Are the module level functions really necessary? Should we deprecate them? As we can convert a VersionInfo object into a string through str(versionobject), it seems to me, they are obsolete...

I see the following benefits of this approach:

Focus on the VersionInfo class, all the magic would happen there. 😉
Avoids writing two functions which mostly implements the same functionality, but with different types.
Simplifies testing.
Focus more on objects than strings (which can't be controlled).
Improves the project to be more slim and well-arranged.
Makes conversion between objects obsolete. Without conversion, it would have a improved effect on performance (maybe only slightly, but...).

If we want to follow this path, we would need:

Use the warning module to issue a deprecation warning in affected module functions.
Document the change for our upcoming major release. 😉
Release 2.9.2 of semver to implement the warnings
Make the whole removal in release 3 only.

@gsakkis, @scls19fr Does that make sense to you? Do I miss an important point? I would really like to hear your view on this. ❤️

The text was updated successfully, but these errors were encountered:

gsakkis · 2020-03-03T11:27:08Z

@tomschr I fully agree on deprecating and eventually removing the module level functions 👍

s-celles · 2020-03-03T11:43:54Z

Removing functions on module level can only be done with major version upgrade (to respect semver). A first step could be to deprecate if there is a general consensus

tomschr · 2020-03-04T13:20:06Z

Maybe I wasn't clear enough, but of course I meant it to remove them in a major release only. If nobody objects. ;))

* Add test cases * In `setup.cfg`, add deprecation warnings filter for pytest * Implement DeprecationWarning with warnings module and the new decorator `deprecated` * Output a DeprecationWarning for the following functions: - semver.parse - semver.parse_version_info - semver.format_version - semver.bump_{major,minor,patch,prerelease,build} - semver.finalize_version - semver.replace

* Add test cases * In `setup.cfg`, add deprecation warnings filter for pytest * Implement DeprecationWarning with warnings module and the new decorator `deprecated` * Output a DeprecationWarning for the following functions: - semver.parse - semver.parse_version_info - semver.format_version - semver.bump_{major,minor,patch,prerelease,build} - semver.finalize_version - semver.replace Add also a deprecation notice in the docstrings of these functions * Update CHANGELOG.rst

tomschr · 2020-03-16T08:15:46Z

Just to clarify the pull request #229: it doesn't remove the modul level function, it lays the foundation so we can remove them easily once we work on major release 3.

* Add test cases * In `setup.cfg`, add deprecation warnings filter for pytest * Implement DeprecationWarning with warnings module and the new decorator `deprecated` * Output a DeprecationWarning for the following functions: - semver.parse - semver.parse_version_info - semver.format_version - semver.bump_{major,minor,patch,prerelease,build} - semver.finalize_version - semver.replace Add also a deprecation notice in the docstrings of these functions * Update CHANGELOG.rst

ppkt · 2020-03-18T22:14:02Z

@tomschr I've mixed feelings about this - while I agree with your points (especially about duplicated code) - I think we should allow using semver in both ways because a lot of software is using module level functions currently.
I checked some popular tools which are using semver: https://libraries.io/pypi/semver/usage:

Enforcing OOP it in next major version is completely fine, but maybe it'll be better to put some extra effort and maintain support for current "users" of module level functions? :)

tomschr · 2020-03-19T06:44:28Z

@ppkt Thanks Karl for your feedback, much appreciated! 👍

we should allow using semver in both ways because a lot of software is using module level functions currently.

This is still the case with the current version. For 2.9.x, it won't change, they will be available. The only difference against previous versions are that the user get a deprecation warning message to use other functions.

Or do you talk about the next major release?

I checked some popular tools which are using semver: https://libraries.io/pypi/semver/usage:

I didn't know that Web page. Very cool, thanks for the link! 👍

maybe it'll be better to put some extra effort and maintain support for current "users" of module level functions? :)

Ok, could you be a bit more verbose what you had in your mind?

ppkt · 2020-03-19T19:01:37Z

Sorry, wasn't clear enough :)

My point is - a lot of software is currently using "module level functions" - by deprecating them (and removing) they might stay with 2.x version. Maybe it's better to refactor code to avoid duplication but leave both ways of working on versions (string-based and object oriented)?

* Add test cases * In `setup.cfg`, add deprecation warnings filter for pytest * Implement DeprecationWarning with warnings module and the new decorator `deprecated` * Output a DeprecationWarning for the following functions: - semver.parse - semver.parse_version_info - semver.format_version - semver.bump_{major,minor,patch,prerelease,build} - semver.finalize_version - semver.replace Add also a deprecation notice in the docstrings of these functions * Update CHANGELOG.rst

tomschr · 2020-03-20T09:58:54Z

@ppkt Oh, I see what you mean. So we should keep them in your opinion.

My point is - a lot of software is currently using "module level functions" - by deprecating them (and removing) they might stay with 2.x version.

Yes, that's a valid point. I have to agree, I have mixed feelings. 😉

On one side, I can fully understand that developers don't want to change a running system. Even if the change would be minor.

On the other side, the change is not that difficult. Change is inevitable. Modules change, programs change, even big operating systems does that. 😉 Of course, the change should be communicated, useful, and there should be a clear migration path.

So... one option could be, to still have the module level functions in place for major release 3, but with all the deprecation warnings. However, I would definitely remove them in major release 4. That should give people really enough time. Of course, it should also be covered by our documentation (I currently lack a good description, I'm aware of that, but haven't had the time).

What do you think of this idea?

* Add test cases * In `setup.cfg`, add deprecation warnings filter for pytest * Implement DeprecationWarning with warnings module and the new decorator `deprecated` * Output a DeprecationWarning for the following functions: - semver.parse - semver.parse_version_info - semver.format_version - semver.bump_{major,minor,patch,prerelease,build} - semver.finalize_version - semver.replace Add also a deprecation notice in the docstrings of these functions * Update CHANGELOG.rst

ppkt · 2020-04-11T08:24:10Z

@tomschr I saw your comment in some other ticket and I completely agree with it - "there should be only one way to do it". I also noticed in your PR is detailed instruction how to replace deprecated functions and I think it's really great.

I also think we should stick with your initial proposal - to deprecate them in 2.9.x and remove in 3.x - there will be a lot of other, potentially breaking, changes in new version so someone who is using our library, will have to check documentation and fix all warnings and errors :)

tomschr · 2020-04-11T09:07:46Z

@ppkt Thanks Karol for your ideas. 👍

Yes, we have now documentation for it. 😉 I've tried to address this and help our users to "migrate". If you have any further ideas, suggestions, or found anything missing, please let me know so I can improve it.

Regarding your change, yes, sure, we can deprecate them in 2.9.x and remove in 3.x. 😉 That's why we discuss this and try to find the best solution.

Could you do me a favor and can do a short test in regards to #229 (comment)? It's about the warning and it drives me nuts. 😄 Maybe I need to read the documentation more carefully.

Thanks Karol! 👍

* Add test cases - Add additional test case for "check" - test_should_process_check_iscalled_with_valid_version - Test also missing finalize_version - Test the warning more thouroughly with pytest.warns instead of just pytest.deprecated_call * In `setup.cfg`, add deprecation warnings filter for pytest * Implement DeprecationWarning with warnings module and the new decorator `deprecated` * Output a DeprecationWarning for the following functions: - semver.bump_{major,minor,patch,prerelease,build} - semver.format_version - semver.finalize_version - semver.parse - semver.parse_version_info - semver.replace - semver.VersionInfo._asdict - semver.VersionInfo._astuple Add also a deprecation notice in the docstrings of these functions * Introduce new public functions: - semver.VersionInfo.to_dict (from former _asdict) - semver.VersionInfo.to_tuple (from former _astuple) - Keep _asdict and _astuple as a (deprecated) function for compatibility reasons * Update CHANGELOG.rst * Update usage documentation: - Add deprecation warning - Explain how to replace deprecated functions - Explain how to display deprecation warnings from semver * Improve documentation of deprecated functions - List deprecated module level functions - Make recommendation and show equivalent code - Mention that deprecated functions will be replaced in semver 3. That means, all deprecated function will be still available in semver 2.x.y. * Move _increment_string into VersionInfo class - Makes removing deprecating functions easier as, for example, bump_prerelease is no longer dependant from an "external" function. - Move _LAST_NUMBER regex into VersionInfo class - Implement _increment_string as a staticmethod Co-authored-by: Karol <karol@ppkt.eu> Co-authored-by: scls19fr <scls19fr@users.noreply.github.com>

* Add test cases - Add additional test case for "check" - test_should_process_check_iscalled_with_valid_version - Test also missing finalize_version - Test the warning more thouroughly with pytest.warns instead of just pytest.deprecated_call * In `setup.cfg`, add deprecation warnings filter for pytest * Implement DeprecationWarning with warnings module and the new decorator `deprecated` * Output a DeprecationWarning for the following functions: - semver.bump_{major,minor,patch,prerelease,build} - semver.format_version - semver.finalize_version - semver.parse - semver.parse_version_info - semver.replace - semver.VersionInfo._asdict - semver.VersionInfo._astuple Add also a deprecation notice in the docstrings of these functions * Introduce new public functions: - semver.VersionInfo.to_dict (from former _asdict) - semver.VersionInfo.to_tuple (from former _astuple) - Keep _asdict and _astuple as a (deprecated) function for compatibility reasons * Update CHANGELOG.rst * Update usage documentation: - Move some information to make them more useful for for the reader - Add deprecation warning - Explain how to replace deprecated functions - Explain how to display deprecation warnings from semver * Improve documentation of deprecated functions - List deprecated module level functions - Make recommendation and show equivalent code - Mention that deprecated functions will be replaced in semver 3. That means, all deprecated function will be still available in semver 2.x.y. * Move _increment_string into VersionInfo class - Makes removing deprecating functions easier as, for example, bump_prerelease is no longer dependant from an "external" function. - Move _LAST_NUMBER regex into VersionInfo class - Implement _increment_string as a staticmethod Co-authored-by: Karol <karol@ppkt.eu> Co-authored-by: scls19fr <scls19fr@users.noreply.github.com>

* Add test cases - Add additional test case for "check" - test_should_process_check_iscalled_with_valid_version - Test also missing finalize_version - Test the warning more thoroughly with pytest.warns instead of just pytest.deprecated_call * In `setup.cfg`, add deprecation warnings filter for pytest * Implement DeprecationWarning with warnings module and the new decorator `deprecated` * Output a DeprecationWarning for the following functions: - semver.bump_{major,minor,patch,prerelease,build} - semver.format_version - semver.finalize_version - semver.parse - semver.parse_version_info - semver.replace - semver.VersionInfo._asdict - semver.VersionInfo._astuple Add also a deprecation notice in the docstrings of these functions * Introduce new public functions: - semver.VersionInfo.to_dict (from former _asdict) - semver.VersionInfo.to_tuple (from former _astuple) - Keep _asdict and _astuple as a (deprecated) function for compatibility reasons * Update CHANGELOG.rst * Update usage documentation: - Move some information to make them more useful for for the reader - Add deprecation warning - Explain how to replace deprecated functions - Explain how to display deprecation warnings from semver * Improve documentation of deprecated functions - List deprecated module level functions - Make recommendation and show equivalent code - Mention that deprecated functions will be replaced in semver 3. That means, all deprecated function will be still available in semver 2.x.y. * Move _increment_string into VersionInfo class - Makes removing deprecating functions easier as, for example, bump_prerelease is no longer dependant from an "external" function. - Move _LAST_NUMBER regex into VersionInfo class - Implement _increment_string as a staticmethod Co-authored-by: Karol <karol@ppkt.eu> Co-authored-by: scls19fr <scls19fr@users.noreply.github.com> Co-authored-by: George Sakkis

tomschr added Question Unclear or open issue subject for debate Enhancement Not a bug, but increases or improves in value, quality, desirability, or attractiveness labels Feb 29, 2020

tomschr mentioned this issue Feb 29, 2020

First implementation of next_version #222

Merged

2 tasks

tomschr mentioned this issue Mar 15, 2020

Deprecate module level functions #229

Merged

tomschr closed this as completed in #229 Apr 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Decide to deprecate module level functions? #225

Decide to deprecate module level functions? #225

tomschr commented Feb 29, 2020 •

edited

Loading

gsakkis commented Mar 3, 2020

Uh oh!

s-celles commented Mar 3, 2020

Uh oh!

tomschr commented Mar 4, 2020 •

edited

Loading

Uh oh!

tomschr commented Mar 16, 2020

Uh oh!

ppkt commented Mar 18, 2020

Uh oh!

tomschr commented Mar 19, 2020

Uh oh!

ppkt commented Mar 19, 2020

Uh oh!

tomschr commented Mar 20, 2020

Uh oh!

ppkt commented Apr 11, 2020

Uh oh!

tomschr commented Apr 11, 2020

Uh oh!

Decide to deprecate module level functions? #225

Decide to deprecate module level functions? #225

Comments

tomschr commented Feb 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Situation

Suggested Solution

gsakkis commented Mar 3, 2020

Uh oh!

s-celles commented Mar 3, 2020

Uh oh!

tomschr commented Mar 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomschr commented Mar 16, 2020

Uh oh!

ppkt commented Mar 18, 2020

Uh oh!

tomschr commented Mar 19, 2020

Uh oh!

ppkt commented Mar 19, 2020

Uh oh!

tomschr commented Mar 20, 2020

Uh oh!

ppkt commented Apr 11, 2020

Uh oh!

tomschr commented Apr 11, 2020

Uh oh!

tomschr commented Feb 29, 2020 •

edited

Loading

tomschr commented Mar 4, 2020 •

edited

Loading