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 #887

Closed
wants to merge 2 commits into from
Closed

Conversation

fra87
Copy link

@fra87 fra87 commented Jul 5, 2022

Added return codes as anticipated in #884

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 fra87 closed this Jul 6, 2022
@fra87 fra87 reopened this Jul 6, 2022
@jmcnamara
Copy link
Owner

I'll admit that I wasn't very enthusiastic about this change, and I am worried that it will break existing check on return values, but you clearly but thought and effort into this so you changed my mind.

I do have quite a few comments to make and if you bear with me through those changes then I'll merge the PR.

@jmcnamara
Copy link
Owner

You will need to squash the two commits into one.

The added return codes are from the workbook and worksheet classes

Fixed SonarCloud issue and warnings
@fra87 fra87 force-pushed the returnCodesToEnum branch from 28affdd to 023d402 Compare July 12, 2022 21:01
@sonarqubecloud
Copy link

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 Jul 12, 2022

I'm happy to hear that; I originally thought it would have been much easier, but there were lots of dependencies (and I wanted to write a complete set of tests).

In any case let me know if there is anything you think is worth modifying.

I squashed the two commits in one, but then there was a conflict. I fixed it properly (actually it was already fixed in the new commit, because the issue was opened before I started working on the modification), but then the merge commit appeared and I'm not sure I can squash also the merge commit. How can this be solved? Can the two commits be pulled, or do you think it is better to start from scratch on a new branch generated from the new master, copy everything from the old branch in the new and then commit?

@jmcnamara
Copy link
Owner

is better to start from scratch on a new branch generated from the new master, copy everything from the old branch in the new and then commit?

I've done that in the past when a project I was trying to submit to had moved on and created a conflict. Feel free to close this PR and submit a new one without conflicts.

@fra87
Copy link
Author

fra87 commented Jul 17, 2022

Closing PR because PR #892 was created (including all the modifications to master up to today)

@fra87 fra87 closed this Jul 17, 2022
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.

2 participants