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

Added unique return codes #892

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fra87
Copy link

@fra87 fra87 commented Jul 17, 2022

Added return codes as anticipated in #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

The added return codes are from the workbook and worksheet classes
@fra87 fra87 mentioned this pull request Jul 17, 2022
@sonarcloud
Copy link

sonarcloud bot commented Jul 17, 2022

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

@fra87
Copy link
Author

fra87 commented Aug 17, 2022

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

@jmcnamara
Copy link
Owner

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.

@wkschwartz
Copy link

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
Copy link

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.

@jmcnamara
Copy link
Owner

@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
Copy link
Owner

jmcnamara commented Nov 19, 2024

I have circled back to this as part of the XlsxWriter v2 refactoring. See the discussion at: Request for Comments: Refactoring of return values to Enums or Exceptions #1100

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