XlsxWriter icon indicating copy to clipboard operation
XlsxWriter copied to clipboard

Added unique return codes

Open fra87 opened this issue 2 years ago • 6 comments

Added return codes as anticipated in https://github.com/jmcnamara/XlsxWriter/issues/884

This pull request is the one including the new modifications to main introduced after PR #887

The return codes are now an Enum inheriting from str, so an explicative message can be automatically printed without helper functions.

The return codes are in file returncodes.py

84 new dedicated tests are added to test/core/test_returncodes.py

Documentation has been updated

At the end I run make test, make test_flake8 and make docs. All tests succeeded and flake8 did not report anything. Development and tests were done with Python 3.9.2

fra87 avatar Jul 17 '22 21:07 fra87

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

sonarqubecloud[bot] avatar Jul 17 '22 21:07 sonarqubecloud[bot]

@jmcnamara do you think this can be merged in the main branch or you reconsidered it?

fra87 avatar Aug 17 '22 12:08 fra87

do you think this can be merged in the main branch or you reconsidered it?

I'm worried about backward compatibility so I really need to consider this one carefully before I merge.

jmcnamara avatar Aug 17 '22 13:08 jmcnamara

One way to maintain backward compatibility would be to replace the Enums with IntEnums.

>>> from enum import IntEnum
>>> class E(IntEnum):
...     A = 0
...     B = -1
...
>>> E.A == 0
True
>>> E.B == -1
True
>>> class F(IntEnum):
...     C = 0
...
>>> F.C == E.A
True
>>> F.C is E.A
False

You can always implement __repr__ or __str__ if you want nice string representations.

wkschwartz avatar Nov 22 '22 15:11 wkschwartz

I should say though that I find the indication of errors by using warnings and return codes instead of raising exceptions makes using xlsxwriter as a library rather difficult. It means I have to catch warnings and sometimes read the text rather than catching exceptions. If you're going to break backward compatibility, I'd suggest considering making more complete use of (ideally highly specific) exception classes.

wkschwartz avatar Nov 22 '22 15:11 wkschwartz

@wkschwartz Thanks for the input.

One way to maintain backward compatibility would be to replace the Enums with IntEnums.

Thanks that is interesting.

If you're going to break backward compatibility, I'd suggest considering making more complete use of (ideally highly specific) exception classes.

Yes, if it was to redo the error handling in XlsxWriter then I would probably ditch the return values and go with Exceptions throughout. When I added Exceptions it was from a viewpoint of things that were actually exceptions and some of the more minor warnings don't really fall into that category but maybe I should have just made all warning an exception.

I don't know if I will revisit that though in the future. I may but I may not.

jmcnamara avatar Nov 22 '22 17:11 jmcnamara